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@xxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: Hannes Reinecke <hare@xxxxxxx>
> > Cc: Johannes Thumshirn <jthumshirn@xxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > ---
> > 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.
