diff mbox series

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

Message ID 20210806074252.398482-3-damien.lemoal@wdc.com
State Superseded
Headers show
Series libata cleanups and improvements | expand

Commit Message

Damien Le Moal Aug. 6, 2021, 7:42 a.m. UTC
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(-)

Comments

James Bottomley Aug. 6, 2021, 2:31 p.m. UTC | #1
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?

James
Damien Le Moal Aug. 6, 2021, 2:33 p.m. UTC | #2
On 2021/08/06 23:31, 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?

Will check.

> 
> James
> 
>
Damien Le Moal Aug. 7, 2021, 3:32 a.m. UTC | #3
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
diff mbox series

Patch

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)
 		have_stop = 1;
 
 	if (have_stop) {