linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"James.Bottomley@HansenPartnership.com" 
	<James.Bottomley@HansenPartnership.com>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
	"martin.petersen@oracle.com" <martin.petersen@oracle.com>
Cc: "sreekanth.reddy@broadcom.com" <sreekanth.reddy@broadcom.com>,
	"suganath-prabu.subramani@broadcom.com" 
	<suganath-prabu.subramani@broadcom.com>,
	"sathya.prakash@broadcom.com" <sathya.prakash@broadcom.com>
Subject: Re: [PATCH v2 2/9] libata: fix ata_host_start()
Date: Sat, 7 Aug 2021 03:32:33 +0000	[thread overview]
Message-ID: <6041d740405be01dc4b1a1f31f723e2e9219aaf6.camel@wdc.com> (raw)
In-Reply-To: <1dca71ad49be897f9544d0de59204e42a5b56a50.camel@HansenPartnership.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


  parent reply	other threads:[~2021-08-07  3:32 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-06  7:42 [PATCH v2 0/9] libata cleanups and improvements Damien Le Moal
2021-08-06  7:42 ` [PATCH v2 1/9] libata: fix ata_host_alloc_pinfo() Damien Le Moal
2021-08-06  8:49   ` Hannes Reinecke
2021-08-06 14:25   ` James Bottomley
2021-08-06 14:31     ` Damien Le Moal
2021-08-06  7:42 ` [PATCH v2 2/9] libata: fix ata_host_start() Damien Le Moal
2021-08-06  8:50   ` Hannes Reinecke
2021-08-06 14:31   ` James Bottomley
2021-08-06 14:33     ` Damien Le Moal
2021-08-07  3:32     ` Damien Le Moal [this message]
2021-08-06  7:42 ` [PATCH v2 3/9] libata: cleanup device sleep capability detection Damien Le Moal
2021-08-06  8:50   ` Hannes Reinecke
2021-08-06  7:42 ` [PATCH v2 4/9] libata: cleanup ata_dev_configure() Damien Le Moal
2021-08-06  9:07   ` Hannes Reinecke
2021-08-06  9:12     ` Damien Le Moal
2021-08-06  9:28       ` Hannes Reinecke
2021-08-06  9:32         ` Damien Le Moal
2021-08-06  7:42 ` [PATCH v2 5/9] libata: cleanup NCQ priority handling Damien Le Moal
2021-08-06  9:09   ` Hannes Reinecke
2021-08-06  7:42 ` [PATCH v2 6/9] libata: fix ata_read_log_page() warning Damien Le Moal
2021-08-06  9:10   ` Hannes Reinecke
2021-08-06  7:42 ` [PATCH v2 7/9] libata: print feature list on device scan Damien Le Moal
2021-08-06  9:11   ` Hannes Reinecke
2021-08-06  7:42 ` [PATCH v2 8/9] libahci: Introduce ncq_prio_supported sysfs sttribute Damien Le Moal
2021-08-06  7:42 ` [PATCH v2 9/9] scsi: mpt3sas: Introduce sas_ncq_prio_supported " Damien Le Moal
2021-08-06  9:12   ` Hannes Reinecke
2021-08-06  9:16     ` Damien Le Moal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6041d740405be01dc4b1a1f31f723e2e9219aaf6.camel@wdc.com \
    --to=damien.lemoal@wdc.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=axboe@kernel.dk \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=sathya.prakash@broadcom.com \
    --cc=sreekanth.reddy@broadcom.com \
    --cc=suganath-prabu.subramani@broadcom.com \
    --subject='Re: [PATCH v2 2/9] libata: fix ata_host_start()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).