Re: [PATCH] scsi_transport_sas: Fix error handling in sas_smp_request()

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Thu, 2017-08-24 at 14:21 +0200, Hannes Reinecke wrote:
> On 08/23/2017 08:25 PM, Bart Van Assche wrote:
> > sas_function_template.smp_handler implementations either return
> > 0 or a Unix error code. Convert that error code into a SCSI
> > result. This patch is what I came up with after having analyzed
> > the following sparse warnings:
> > 
> > drivers/scsi/scsi_transport_sas.c:187:21: warning: incorrect type in assignment (different base types)
> > drivers/scsi/scsi_transport_sas.c:187:21:    expected restricted blk_status_t [usertype] ret
> > drivers/scsi/scsi_transport_sas.c:187:21:    got int
> > drivers/scsi/scsi_transport_sas.c:188:39: warning: incorrect type in assignment (different base types)
> > drivers/scsi/scsi_transport_sas.c:188:39:    expected int [signed] result
> > drivers/scsi/scsi_transport_sas.c:188:39:    got restricted blk_status_t [usertype] ret
> > 
> > Fixes: commit 17d5363b83f8 ("scsi: introduce a result field in struct scsi_request")
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Johannes Thumshirn <jthumshirn@suse.de>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  drivers/scsi/scsi_transport_sas.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> > index 5006a656e16a..a318c46db7cc 100644
> > --- a/drivers/scsi/scsi_transport_sas.c
> > +++ b/drivers/scsi/scsi_transport_sas.c
> > @@ -173,7 +173,7 @@ static void sas_smp_request(struct request_queue *q, struct Scsi_Host *shost,
> >  			    struct sas_rphy *rphy)
> >  {
> >  	struct request *req;
> > -	blk_status_t ret;
> > +	int ret;
> >  	int (*handler)(struct Scsi_Host *, struct sas_rphy *, struct request *);
> >  
> >  	while ((req = blk_fetch_request(q)) != NULL) {
> > @@ -185,7 +185,9 @@ static void sas_smp_request(struct request_queue *q, struct Scsi_Host *shost,
> >  				blk_rq_bytes(req->next_rq);
> >  		handler = to_sas_internal(shost->transportt)->f->smp_handler;
> >  		ret = handler(shost, rphy, req);
> > -		scsi_req(req)->result = ret;
> > +		WARN_ONCE(ret != 0 && !IS_ERR_VALUE(ret + 0UL),
> > +			  "%s: ret = %d\n", __func__, ret);
> > +		scsi_req(req)->result = ret ? DID_ERROR << 16 : 0;
> >  
> >  		blk_end_request_all(req, 0);
> >  
> > 
> 
> Weelll ... I'd rather audit the handler so as to ensure that the correct
> value is returned.
> And this 'ret + 0UL' construct is decidedly ugly ...

Hello Hannes,

Changing "+ 0UL" into an explicit (unsigned long) cast is easy. But I would
prefer to leave the conversion of the smp_handler functions to someone who
has the hardware available to test such a conversion. These are the smp_handler
implementations I am aware of:

$ git grep -nH '\.smp_handler[[:blank:]]*='
drivers/message/fusion/mptsas.c:2356:	.smp_handler		= mptsas_smp_handler,
drivers/scsi/hpsa.c:9463:	.smp_handler = hpsa_sas_smp_handler,
drivers/scsi/libsas/sas_init.c:548:	.smp_handler = sas_smp_handler,
drivers/scsi/mpt3sas/mpt3sas_transport.c:2129:	.smp_handler		= _transport_smp_handler,
drivers/scsi/smartpqi/smartpqi_sas_transport.c:349:	.smp_handler = pqi_sas_smp_handler,

Bart.