On 08/24/2017 06:06 PM, Bart Van Assche wrote:
> 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,
>
Yeah, and none of them work properly.
Johannes tried to reconcile the scsi_transport_fc and bsg_lib bsg
implementation, but then got stuck and didn't pursue it further.
So I would love to see some cleanup here.
And yes, I guess we can test things here.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)