Re: [PATCH v2 2/9] libata: fix ata_host_start()

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On Fri, 2021-08-06 at 07:31 -0700, James Bottomley wrote:
> On Fri, 2021-08-06 at 16:42 +0900, Damien Le Moal wrote:
> > The loop on entry of ata_host_start() may not initialize host->ops to
> > a non NULL value. The test on the host_stop field of host->ops must
> > then be preceded by a check that host->ops is not NULL.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> > ---
> >  drivers/ata/libata-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index ea8b91297f12..fe49197caf99 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5573,7 +5573,7 @@ int ata_host_start(struct ata_host *host)
> >  			have_stop = 1;
> >  	}
> >  
> > -	if (host->ops->host_stop)
> > +	if (host->ops && host->ops->host_stop)
> 
> since have_stop was already set by the port ops, surely this entire if
> is redundant?

Having another look at this, I do not think so.
The loop preceding the if is:

	for (i = 0; i < host->n_ports; i++) {
		struct ata_port *ap = host->ports[i];

		ata_finalize_port_ops(ap->ops);

		if (!host->ops && !ata_port_is_dummy(ap))
			host->ops = ap->ops;

		if (ap->ops->port_stop)
			have_stop = 1;
	}

So have_stop is set based on the port_stop operation of the port. The "if"
tests the host operation host_stop, so not the same thing.

The kernel bot warnings I got complain about the fact that the host ops may not
be set by the loop and can be null if the host ops are not already set and all
ports are dummy ones. In practice, I do not think that can happen. The patch
does not change anything and will only silence static checkers.

> 
> James
> 
> 

-- 
Damien Le Moal
Western Digital Research