[patch] [SCSI] scsi_transport_sas: check for allocation failure

[patch] [SCSI] scsi_transport_sas: check for allocation failure

[Dan Carpenter]

Static checkers complain that this allocation isn't checked.  We
should return zero if the allocation fails.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 1b68142..a022997 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -379,9 +379,12 @@ sas_tlr_supported(struct scsi_device *sdev)
 {
 	const int vpd_len = 32;
 	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
-	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
+	char *buffer;
 	int ret = 0;
 
+	buffer = kzalloc(vpd_len, GFP_KERNEL);
+	if (!buffer)
+		goto out;
 	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
 		goto out;
 

Re: [patch] [SCSI] scsi_transport_sas: check for allocation failure

[Douglas Gilbert]

On 13-03-08 07:02 AM, Dan Carpenter wrote:
> Static checkers complain that this allocation isn't checked.  We
> should return zero if the allocation fails.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 1b68142..a022997 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -379,9 +379,12 @@ sas_tlr_supported(struct scsi_device *sdev)
>   {
>   	const int vpd_len = 32;
>   	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
> -	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
> +	char *buffer;
>   	int ret = 0;
>
> +	buffer = kzalloc(vpd_len, GFP_KERNEL);
> +	if (!buffer)
> +		goto out;
>   	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
>   		goto out;
>

For 32 bytes, why not use the stack?

unsigned int
sas_tlr_supported(struct scsi_device *sdev)
{
         unsigned char buffer[32];
         struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
         int ret = 0;

         if (scsi_get_vpd_page(sdev, 0x90, buffer, sizeof(buffer)))
                 goto out;

         /*
          * The VPD Protocol Specific Logical Unit page (0x90) for SAS
          * has a 4 byte header and then one descriptor per device port.
          * The TLR bit is at offset 8 on each port descriptor.
          * We take the TLR value in the first descriptor.
          */
         ret = buffer[4 + 8] & 0x01;

  out:
         rdev->tlr_supported = ret;
         return ret;
}


Note the comment is changed.

Doug Gilbert

Re: [patch] [SCSI] scsi_transport_sas: check for allocation failure

[Dan Carpenter]

On Fri, Mar 08, 2013 at 12:57:12PM -0500, Douglas Gilbert wrote:
> On 13-03-08 07:02 AM, Dan Carpenter wrote:
> 
> For 32 bytes, why not use the stack?
> 

It's a good point.  I'll resend on Monday.

regards,
dan carpenter

Re: [patch] [SCSI] scsi_transport_sas: check for allocation failure

[James Bottomley]

On Fri, 2013-03-08 at 12:57 -0500, Douglas Gilbert wrote:
> On 13-03-08 07:02 AM, Dan Carpenter wrote:
> > Static checkers complain that this allocation isn't checked.  We
> > should return zero if the allocation fails.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> > index 1b68142..a022997 100644
> > --- a/drivers/scsi/scsi_transport_sas.c
> > +++ b/drivers/scsi/scsi_transport_sas.c
> > @@ -379,9 +379,12 @@ sas_tlr_supported(struct scsi_device *sdev)
> >   {
> >   	const int vpd_len = 32;
> >   	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
> > -	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
> > +	char *buffer;
> >   	int ret = 0;
> >
> > +	buffer = kzalloc(vpd_len, GFP_KERNEL);
> > +	if (!buffer)
> > +		goto out;
> >   	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
> >   		goto out;
> >
> 
> For 32 bytes, why not use the stack?

Because the buffer is a DMA target.  You can't DMA to stack because of
padding and cacheline issues.

James

Re: [patch] [SCSI] scsi_transport_sas: check for allocation failure

[Douglas Gilbert]

On 13-03-08 05:50 PM, James Bottomley wrote:
> On Fri, 2013-03-08 at 12:57 -0500, Douglas Gilbert wrote:
>> On 13-03-08 07:02 AM, Dan Carpenter wrote:
>>> Static checkers complain that this allocation isn't checked.  We
>>> should return zero if the allocation fails.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
>>> index 1b68142..a022997 100644
>>> --- a/drivers/scsi/scsi_transport_sas.c
>>> +++ b/drivers/scsi/scsi_transport_sas.c
>>> @@ -379,9 +379,12 @@ sas_tlr_supported(struct scsi_device *sdev)
>>>    {
>>>    	const int vpd_len = 32;
>>>    	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
>>> -	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
>>> +	char *buffer;
>>>    	int ret = 0;
>>>
>>> +	buffer = kzalloc(vpd_len, GFP_KERNEL);
>>> +	if (!buffer)
>>> +		goto out;
>>>    	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
>>>    		goto out;
>>>
>>
>> For 32 bytes, why not use the stack?
>
> Because the buffer is a DMA target.  You can't DMA to stack because of
> padding and cacheline issues.

And I went to the definition of scsi_get_vpd_page()
to see if that was called out in the header comments.
Guess what ... and those same header comments talked
about freeing a returned pointer. It needs to be
cleaned up, IMO.

Doug Gilbert

/**
  * scsi_get_vpd_page - Get Vital Product Data from a SCSI device
  * @sdev: The device to ask
  * @page: Which Vital Product Data to return
  * @buf: where to store the VPD
  * @buf_len: number of bytes in the VPD buffer area
  *
  * SCSI devices may optionally supply Vital Product Data.  Each 'page'
  * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
  * If the device supports this VPD page, this routine returns a pointer
  * to a buffer containing the data from that page.  The caller is
  * responsible for calling kfree() on this pointer when it is no longer
  * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
  */
int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
                       int buf_len)

Re: [patch] [SCSI] scsi_transport_sas: check for allocation failure

[Dan Carpenter]

On Fri, Mar 08, 2013 at 10:50:19PM +0000, James Bottomley wrote:
> On Fri, 2013-03-08 at 12:57 -0500, Douglas Gilbert wrote:
> > On 13-03-08 07:02 AM, Dan Carpenter wrote:
> > > Static checkers complain that this allocation isn't checked.  We
> > > should return zero if the allocation fails.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> > > index 1b68142..a022997 100644
> > > --- a/drivers/scsi/scsi_transport_sas.c
> > > +++ b/drivers/scsi/scsi_transport_sas.c
> > > @@ -379,9 +379,12 @@ sas_tlr_supported(struct scsi_device *sdev)
> > >   {
> > >   	const int vpd_len = 32;
> > >   	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
> > > -	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
> > > +	char *buffer;
> > >   	int ret = 0;
> > >
> > > +	buffer = kzalloc(vpd_len, GFP_KERNEL);
> > > +	if (!buffer)
> > > +		goto out;
> > >   	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
> > >   		goto out;
> > >
> > 
> > For 32 bytes, why not use the stack?
> 
> Because the buffer is a DMA target.  You can't DMA to stack because of
> padding and cacheline issues.
> 

I think stack data works here.  scsi_execute() calls
blk_rq_map_kern() which handles stack memory and alignment issues.

regards,
dan carpenter

Re: [patch] [SCSI] scsi_transport_sas: check for allocation failure

[Douglas Gilbert]

On 13-03-11 09:10 AM, Dan Carpenter wrote:
> On Fri, Mar 08, 2013 at 10:50:19PM +0000, James Bottomley wrote:
>> On Fri, 2013-03-08 at 12:57 -0500, Douglas Gilbert wrote:
>>> On 13-03-08 07:02 AM, Dan Carpenter wrote:
>>>> Static checkers complain that this allocation isn't checked.  We
>>>> should return zero if the allocation fails.
>>>>
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>
>>>> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
>>>> index 1b68142..a022997 100644
>>>> --- a/drivers/scsi/scsi_transport_sas.c
>>>> +++ b/drivers/scsi/scsi_transport_sas.c
>>>> @@ -379,9 +379,12 @@ sas_tlr_supported(struct scsi_device *sdev)
>>>>    {
>>>>    	const int vpd_len = 32;
>>>>    	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
>>>> -	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
>>>> +	char *buffer;
>>>>    	int ret = 0;
>>>>
>>>> +	buffer = kzalloc(vpd_len, GFP_KERNEL);
>>>> +	if (!buffer)
>>>> +		goto out;
>>>>    	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
>>>>    		goto out;
>>>>
>>>
>>> For 32 bytes, why not use the stack?
>>
>> Because the buffer is a DMA target.  You can't DMA to stack because of
>> padding and cacheline issues.
>>
>
> I think stack data works here.  scsi_execute() calls
> blk_rq_map_kern() which handles stack memory and alignment issues.

That being the case, several other callers of
scsi_get_vpd_page() 9and friends) could be
simplified and sped up.

Also since VPD pages don't change and they can carry
a lot of disparate information (e.g. the Extended
Inquiry and Block Limits pages) perhaps they could
be cached by the appropriate level (e.g. Extended
Inquiry cached by mid-level; Block Limits cached
by sd driver).

Doug Gilbert

Re: [patch] [SCSI] scsi_transport_sas: check for allocation failure

[James Bottomley]

On Mon, 2013-03-11 at 10:48 -0400, Douglas Gilbert wrote:
> On 13-03-11 09:10 AM, Dan Carpenter wrote:
> > On Fri, Mar 08, 2013 at 10:50:19PM +0000, James Bottomley wrote:
> >> On Fri, 2013-03-08 at 12:57 -0500, Douglas Gilbert wrote:
> >>> On 13-03-08 07:02 AM, Dan Carpenter wrote:
> >>>> Static checkers complain that this allocation isn't checked.  We
> >>>> should return zero if the allocation fails.
> >>>>
> >>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>>>
> >>>> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> >>>> index 1b68142..a022997 100644
> >>>> --- a/drivers/scsi/scsi_transport_sas.c
> >>>> +++ b/drivers/scsi/scsi_transport_sas.c
> >>>> @@ -379,9 +379,12 @@ sas_tlr_supported(struct scsi_device *sdev)
> >>>>    {
> >>>>    	const int vpd_len = 32;
> >>>>    	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
> >>>> -	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
> >>>> +	char *buffer;
> >>>>    	int ret = 0;
> >>>>
> >>>> +	buffer = kzalloc(vpd_len, GFP_KERNEL);
> >>>> +	if (!buffer)
> >>>> +		goto out;
> >>>>    	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
> >>>>    		goto out;
> >>>>
> >>>
> >>> For 32 bytes, why not use the stack?
> >>
> >> Because the buffer is a DMA target.  You can't DMA to stack because of
> >> padding and cacheline issues.
> >>
> >
> > I think stack data works here.  scsi_execute() calls
> > blk_rq_map_kern() which handles stack memory and alignment issues.
> 
> That being the case, several other callers of
> scsi_get_vpd_page() 9and friends) could be
> simplified and sped up.

Um, I'm not sure you'll speed stuff up by doing this: have you seen what
bio_copy_kern() does?  it will allocate a page per iov entry (in this
case probably only a single entry) and copy the data into and out of
that page, so not only do we get the copy overhead. We also get page
allocation overheads instead of the small number of bytes we might need,
which could cause quite a stall in a low memory situation.

I'm not saying don't do it, I'm just saying think of the consequences
over the fiddle of doing a small kmalloc.

> Also since VPD pages don't change and they can carry
> a lot of disparate information (e.g. the Extended
> Inquiry and Block Limits pages) perhaps they could
> be cached by the appropriate level (e.g. Extended
> Inquiry cached by mid-level; Block Limits cached
> by sd driver).

We cache the ones we care about, but we could always add more if there's
enough call, I suppose.

James