Re: [PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[Niklas Cassel <cassel@kernel.org>]

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