| Message ID | 20210806074252.398482-3-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libata cleanups and improvements | expand |
On 8/6/21 9:42 AM, 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) > have_stop = 1; > > if (have_stop) { > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
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
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 > >
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 > >
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) {
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(-)