| Message ID | 20240626180031.4050226-15-cassel@kernel.org |
|---|---|
| Headers | show |
| Series | ata,libsas: Assign the unique id used for printing earlier | expand |
On Wed, Jun 26, 2024 at 08:00:37PM +0200, Niklas Cassel wrote: > @@ -5908,12 +5903,13 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh > return -EINVAL; > } > > - /* Blow away unused ports. This happens when LLD can't > - * determine the exact number of ports to allocate at > - * allocation time. > + /* > + * For a driver using ata_host_register(), the ports are allocated by > + * ata_host_alloc(), which also allocates the host->ports array. > + * The number of array elements must match host->n_ports. > */ > for (i = host->n_ports; host->ports[i]; i++) > - kfree(host->ports[i]); > + WARN_ON(host->ports[i]); Nit: Even though we replace the kfree() with a WARN_ON() here, the strictly correct thing would have been for the earlier patch in this series: "ata,scsi: libata-core: Add ata_port_free()" to have replaced the kfree() with ata_port_free(), and then for this patch to replace the ata_port_free() with a WARN_ON(). Kind regards, Niklas
On 6/27/24 03:00, Niklas Cassel wrote: > Currently, the ata_port print_ids are increased indefinitely, even when > there are lower ids available. > > E.g. on first boot you will have ata1-ata6 assigned. > After a rmmod + modprobe, you will instead have ata7-ata12 assigned. > > Move to use the ida_alloc() API, such that print_ids will get reused. > This means that even after a rmmod + modprobe, the ports will be assigned > print_ids ata1-ata6. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Looks good. But maybe it would make sense to squash this together with patch 10 ? Either way, Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
On 26/06/2024 19:00, Niklas Cassel wrote: > Hello all, > > This patch series was orginally meant to simply assign a unique id used > for printing earlier (ap->print_id), but has since grown to also include > cleanups related to ata_port_alloc() (since ap->print_id is now assigned > in ata_port_alloc()). > There's no real problem statement wrt print_id, telling how and why things are like they are, how it is a problem, and how it is improved in this series. > Patch 1-3 fixes incorrect cleanups in the error paths. > Patch 4,12 removes a useless libata wrappers only used for libsas. > Patch 5 introduces a ata_port_free(), in order to avoid duplicated code. > Patch 6 removes a unused function declaration in include/linux/libata.h. > Patch 7 remove support for decreasing the number of ports, as it is never > used by any libata driver (including libsas and ipr). > Patch 8 removes a superfluous assignment in ata_sas_port_alloc(). > Patch 9 removes the unnecessary local_port_no struct member in ata_port. > Patch 10 performs the ata_port print_id assignment earlier, so that the > ata_port_* print functions can be used even before the ata_host > has been registered. > Patch 11 changes the print_id assignment to use an ida_alloc(), such that > we will reuse IDs that are no longer in use, rather than keep > increasing the print_id forever. > Patch 13 adds a debug print in case the port is marked as external, this > code runs before the ata_host has been registered, so it depends > on patch 10.
On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote: > On 26/06/2024 19:00, Niklas Cassel wrote: > > Hello all, > > > > This patch series was orginally meant to simply assign a unique id used > > for printing earlier (ap->print_id), but has since grown to also include > > cleanups related to ata_port_alloc() (since ap->print_id is now assigned > > in ata_port_alloc()). > > > > There's no real problem statement wrt print_id, telling how and why things > are like they are, how it is a problem, and how it is improved in this > series. You are right, it is missing from the cover-letter. It was there in v1: https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/ """ This series moves the assignment of ap->print_id, which is used as a unique id for each port, earlier, such that we can use the ata_port_* print functions even before the ata_host has been registered. """ Will re-add it in v3. Kind regards, Niklas
On 27/06/2024 13:32, Niklas Cassel wrote: > On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote: >> On 26/06/2024 19:00, Niklas Cassel wrote: >>> Hello all, >>> >>> This patch series was orginally meant to simply assign a unique id used >>> for printing earlier (ap->print_id), but has since grown to also include >>> cleanups related to ata_port_alloc() (since ap->print_id is now assigned >>> in ata_port_alloc()). >>> >> >> There's no real problem statement wrt print_id, telling how and why things >> are like they are, how it is a problem, and how it is improved in this >> series. > > You are right, it is missing from the cover-letter. > > It was there in v1: > https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/ > > """ > This series moves the assignment of ap->print_id, which is used as a > unique id for each port, earlier, such that we can use the ata_port_* > print functions even before the ata_host has been registered. > """ OK, fine. I see code which checks vs ap->print_id, like: static void ata_force_link_limits(struct ata_link *link) { ... if (fe->port != -1 && fe->port != link->ap->print_id) continue; Is this all ok to deal with this print_id assignment change? To me, it seems natural to assign a valid print_id from the alloc time, so I can't help but wonder it was done the current way. Thanks, John
On Thu, Jun 27, 2024 at 01:54:34PM +0100, John Garry wrote: > On 27/06/2024 13:32, Niklas Cassel wrote: > > On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote: > > > On 26/06/2024 19:00, Niklas Cassel wrote: > > > > Hello all, > > > > > > > > This patch series was orginally meant to simply assign a unique id used > > > > for printing earlier (ap->print_id), but has since grown to also include > > > > cleanups related to ata_port_alloc() (since ap->print_id is now assigned > > > > in ata_port_alloc()). > > > > > > > > > > There's no real problem statement wrt print_id, telling how and why things > > > are like they are, how it is a problem, and how it is improved in this > > > series. > > > > You are right, it is missing from the cover-letter. > > > > It was there in v1: > > https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/ > > > > """ > > This series moves the assignment of ap->print_id, which is used as a > > unique id for each port, earlier, such that we can use the ata_port_* > > print functions even before the ata_host has been registered. > > """ > > OK, fine. > > I see code which checks vs ap->print_id, like: > > static void ata_force_link_limits(struct ata_link *link) > { > ... > if (fe->port != -1 && fe->port != link->ap->print_id) > continue; > > > Is this all ok to deal with this print_id assignment change? > > To me, it seems natural to assign a valid print_id from the alloc time, so I > can't help but wonder it was done the current way. ap->print_id was assigned after calling ata_host_register(), because libata allowed a driver that did not know how many ports it had, to initially call ata_alloc_host() with a big number of ports, and then reduce the host->n_ports variable once it knew the actually number of ports, before calling ata_host_register(), which would then free the "excess" ports. This feature has actually never been used by and driver, and I remove support for this in this series: https://lore.kernel.org/linux-ide/20240626180031.4050226-22-cassel@kernel.org/ However, you do raise a good point... ap->print_id is just supposed to be used for printing, but it appears that ata_force_link_limits() and some other ata_force_*() functions make use of it for other things... sigh... Hopefully I can just change them from: if (fe->port != -1 && fe->port != link->ap->print_id) to if (fe->port != -1) but I will need to look in to this further... Thank you for noticing this (ab)use of print_id! Kind regards, Niklas
On Thu, Jun 27, 2024 at 05:07:43PM +0200, Niklas Cassel wrote: > On Thu, Jun 27, 2024 at 01:54:34PM +0100, John Garry wrote: > > On 27/06/2024 13:32, Niklas Cassel wrote: > > > On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote: > > > > On 26/06/2024 19:00, Niklas Cassel wrote: > > > > > Hello all, > > > > > > > > > > This patch series was orginally meant to simply assign a unique id used > > > > > for printing earlier (ap->print_id), but has since grown to also include > > > > > cleanups related to ata_port_alloc() (since ap->print_id is now assigned > > > > > in ata_port_alloc()). > > > > > > > > > > > > > There's no real problem statement wrt print_id, telling how and why things > > > > are like they are, how it is a problem, and how it is improved in this > > > > series. > > > > > > You are right, it is missing from the cover-letter. > > > > > > It was there in v1: > > > https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/ > > > > > > """ > > > This series moves the assignment of ap->print_id, which is used as a > > > unique id for each port, earlier, such that we can use the ata_port_* > > > print functions even before the ata_host has been registered. > > > """ > > > > OK, fine. > > > > I see code which checks vs ap->print_id, like: > > > > static void ata_force_link_limits(struct ata_link *link) > > { > > ... > > if (fe->port != -1 && fe->port != link->ap->print_id) > > continue; > > > > > > Is this all ok to deal with this print_id assignment change? > > > > To me, it seems natural to assign a valid print_id from the alloc time, so I > > can't help but wonder it was done the current way. > > ap->print_id was assigned after calling ata_host_register(), because libata > allowed a driver that did not know how many ports it had, to initially call > ata_alloc_host() with a big number of ports, and then reduce the host->n_ports > variable once it knew the actually number of ports, before calling > ata_host_register(), which would then free the "excess" ports. > > This feature has actually never been used by and driver, and I remove support > for this in this series: > https://lore.kernel.org/linux-ide/20240626180031.4050226-22-cassel@kernel.org/ > > > However, you do raise a good point... > ap->print_id is just supposed to be used for printing, but it appears that > ata_force_link_limits() and some other ata_force_*() functions make use of > it for other things... sigh... > > Hopefully I can just change them from: > if (fe->port != -1 && fe->port != link->ap->print_id) > to > if (fe->port != -1) > > but I will need to look in to this further... So, looking more closely at this, the code is actually not abusing print_id. Looking at libata.force in Documentation/admin-guide/kernel-parameters.txt: [LIBATA] Force configurations. The format is a comma- separated list of "[ID:]VAL" where ID is PORT[.DEVICE]. PORT and DEVICE are decimal numbers matching port, link or device. Basically, it matches the ATA ID string printed on console by libata. While this seems a bit fragile, since it relies on the probe ordering of the SATA controller drivers, which could change, it does still work as designed after this series: I added the following to my kernel command line: "libata.force=5:nolpm" which yielded: [ 1.811464] ata3.00: FORCE: horkage modified (nolpm) [ 1.811466] ata3.00: LPM support broken, forcing max_power [ 1.811468] ata3.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100 [ 1.811470] ata3.00: 2097152 sectors, multi 16: LBA48 NCQ (depth 32) [ 1.811474] ata3.00: applying bridge limits [ 1.811535] ata3.00: LPM support broken, forcing max_power [ 1.811537] ata3.00: configured for UDMA/100 And considering that all checks against ap->print_id is for libata.force related parameters, I think that we are all good. Kind regards, Niklas
On Thu, Jun 27, 2024 at 10:37:40AM +0900, Damien Le Moal wrote: > On 6/27/24 03:00, Niklas Cassel wrote: > > Currently, the ata_port print_ids are increased indefinitely, even when > > there are lower ids available. > > > > E.g. on first boot you will have ata1-ata6 assigned. > > After a rmmod + modprobe, you will instead have ata7-ata12 assigned. > > > > Move to use the ida_alloc() API, such that print_ids will get reused. > > This means that even after a rmmod + modprobe, the ports will be assigned > > print_ids ata1-ata6. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > Looks good. But maybe it would make sense to squash this together with patch 10 ? Patch 10 initializes the print_ids earlier (which is a perfectly fine change on its own, even with the old way to assign print_ids), while this patch changes for the print_ids to be reusable. I think that logically, it is two different logical changes so I will keep them as separate patches in v3. Kind regards, Niklas
Hello all, This patch series was orginally meant to simply assign a unique id used for printing earlier (ap->print_id), but has since grown to also include cleanups related to ata_port_alloc() (since ap->print_id is now assigned in ata_port_alloc()). Patch 1-3 fixes incorrect cleanups in the error paths. Patch 4,12 removes a useless libata wrappers only used for libsas. Patch 5 introduces a ata_port_free(), in order to avoid duplicated code. Patch 6 removes a unused function declaration in include/linux/libata.h. Patch 7 remove support for decreasing the number of ports, as it is never used by any libata driver (including libsas and ipr). Patch 8 removes a superfluous assignment in ata_sas_port_alloc(). Patch 9 removes the unnecessary local_port_no struct member in ata_port. Patch 10 performs the ata_port print_id assignment earlier, so that the ata_port_* print functions can be used even before the ata_host has been registered. Patch 11 changes the print_id assignment to use an ida_alloc(), such that we will reuse IDs that are no longer in use, rather than keep increasing the print_id forever. Patch 13 adds a debug print in case the port is marked as external, this code runs before the ata_host has been registered, so it depends on patch 10. Martin, how do you want us to coordinate libsas changes? You don't seem to have any libsas changes staged for 6.11 so far, and the libsas changes in this series are quite isolated (and small), so perhaps we can simply queue them via the libata tree? Kind regards, Niklas Changes since v1: -Added patches that fixes incorrect cleanups in the error paths. -Added patches to remove useless libata wrappers only used for libsas. -Added patch that introduces ata_port_free(). -Added patch that removes a unused function declaration in libata.h. -Added patch that removes local_port_no (Damien). -Added patch that assigns the print_id using ida_alloc() (Damien). -Picked up tags. Link to v1: https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/ Niklas Cassel (13): ata: libata-core: Fix null pointer dereference on error ata: libata-core: Fix double free on error ata: ahci: Clean up sysfs file on error ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}() ata,scsi: libata-core: Add ata_port_free() ata: libata: Remove unused function declaration for ata_scsi_detect() ata: libata-core: Remove support for decreasing the number of ports ata: libata-sata: Remove superfluous assignment in ata_sas_port_alloc() ata: libata-core: Remove local_port_no struct member ata: libata: Assign print_id at port allocation time ata: libata-core: Reuse available ata_port print_ids ata,scsi: Remove useless ata_sas_port_alloc() wrapper ata: ahci: Add debug print for external port drivers/ata/ahci.c | 21 +++++++--- drivers/ata/libata-core.c | 66 ++++++++++++++++-------------- drivers/ata/libata-sata.c | 49 ---------------------- drivers/ata/libata-transport.c | 5 ++- drivers/ata/libata-transport.h | 3 -- drivers/ata/libata.h | 1 - drivers/scsi/libsas/sas_ata.c | 14 +++++-- drivers/scsi/libsas/sas_discover.c | 4 +- include/linux/libata.h | 12 +++--- 9 files changed, 71 insertions(+), 104 deletions(-)