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@xxxxxxxxxx/ > > > > """ > > 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@xxxxxxxxxx/ 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
