Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry
On 01/31/14 10:29, Hannes Reinecke wrote:
> - len = (h->buff[2] << 8) + h->buff[3] + 4;
> - if (len > h->bufflen) {
> + len = (buff[2] << 8) + buff[3] + 4;
> + if (len > bufflen) {
I think this code will become easier to read when using
get_unaligned_be16() for extracting the length from the VPD response.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry
On 02/13/2014 10:51 AM, Maurizio Lombardi wrote:
> Hi,
>
> On 01/31/2014 10:29 AM, Hannes Reinecke wrote:
>> static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data
>> *h)
>> {
>> +unsigned char *buff;
>> +unsigned char bufflen = 36;
>> int len, timeout = ALUA_FAILOVER_TIMEOUT;
> [...]
>> +len = (buff[2] << 8) + buff[3] + 4;
>> +if (len > bufflen) {
> [...]
>> +bufflen = len;
>
> just a nit: is it safe to use char as the type of bufflen? Isn't better
> to declare it as int just in case len is > than 255 ?
>
Yes, true.
However, this whole section needs to be reworked anyway, as there's
a fair chance we're getting the VPD page 0x83 for free.
(Cf my latest patchset 'Display EVPD pages in sysfs').
And jejb made some positive noises that, so I'll be reworking the
scsi_dh_alua patchset to take advantage of the new fields.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry
Hi,
On 01/31/2014 10:29 AM, Hannes Reinecke wrote:
> static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
> {
> + unsigned char *buff;
> + unsigned char bufflen = 36;
> int len, timeout = ALUA_FAILOVER_TIMEOUT;
[...]
>+ len = (buff[2] << 8) + buff[3] + 4;
>+ if (len > bufflen) {
[...]
>+ bufflen = len;
just a nit: is it safe to use char as the type of bufflen? Isn't better
to declare it as int just in case len is > than 255 ?
Regards,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry
On 01/17/2014 10:04 AM, Mike Christie wrote:
> On 12/20/2013 06:13 AM, Hannes Reinecke wrote:
>> VPD inquiry need to be done only once, so we can be using
>> a local buffer here.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> drivers/scsi/device_handler/scsi_dh_alua.c | 45
>> ++
>> 1 file changed, 27 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
>> b/drivers/scsi/device_handler/scsi_dh_alua.c
>> index adc77ef..49952f4 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -322,16 +322,27 @@ static int alua_check_tpgs(struct scsi_device *sdev,
>> struct alua_dh_data *h)
>> */
>> static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data
>> *h)
>> {
>> +unsigned char *buff;
>> +unsigned char bufflen = 36;
>> int len, timeout = ALUA_FAILOVER_TIMEOUT;
>> unsigned char sense[SCSI_SENSE_BUFFERSIZE];
>> struct scsi_sense_hdr sense_hdr;
>> unsigned retval;
>> unsigned char *d;
>> unsigned long expiry;
>> +int err;
>>
>> expiry = round_jiffies_up(jiffies + timeout);
>> retry:
>> -retval = submit_vpd_inquiry(sdev, h->buff, h->bufflen, sense);
>> +buff = kmalloc(bufflen, GFP_ATOMIC);
>> +if (!buff) {
>>
>
> Why GFP_ATOMIC? I think it can be less restrictive in this path. If you
> need GFP_ATOMIC here, then there are places in this code path you would
> want to change from GFP_KERNEL to GFP_ATOMIC.
>
Yeah, all right. This is by no means time-critical.
I'll be fixing up the patch.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry
On 12/20/2013 06:13 AM, Hannes Reinecke wrote:
> VPD inquiry need to be done only once, so we can be using
> a local buffer here.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/device_handler/scsi_dh_alua.c | 45
> ++
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> b/drivers/scsi/device_handler/scsi_dh_alua.c
> index adc77ef..49952f4 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -322,16 +322,27 @@ static int alua_check_tpgs(struct scsi_device *sdev,
> struct alua_dh_data *h)
> */
> static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
> {
> + unsigned char *buff;
> + unsigned char bufflen = 36;
> int len, timeout = ALUA_FAILOVER_TIMEOUT;
> unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> struct scsi_sense_hdr sense_hdr;
> unsigned retval;
> unsigned char *d;
> unsigned long expiry;
> + int err;
>
> expiry = round_jiffies_up(jiffies + timeout);
> retry:
> - retval = submit_vpd_inquiry(sdev, h->buff, h->bufflen, sense);
> + buff = kmalloc(bufflen, GFP_ATOMIC);
> + if (!buff) {
>
Why GFP_ATOMIC? I think it can be less restrictive in this path. If you
need GFP_ATOMIC here, then there are places in this code path you would
want to change from GFP_KERNEL to GFP_ATOMIC.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
