Re: [PATCH bluetooth-next] ieee802154/cc2520: check for return values in cc2520_filter()

[Stefan Schmidt <stefan@osg.samsung.com>]

Hello.

On 09/06/15 11:21, Varka Bhadram wrote:
> Hi Stefan,
>
> On 06/09/2015 02:41 PM, Stefan Schmidt wrote:
>
>> neither ram nor register write return values have been checked here. 
>> Checking
>> both now. Assign ret with 0 as all other assignments are inside if 
>> blocks and
>> might not happen before we return ret.
>>
>> CID: 1230469
>> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
>> Cc: Varka Bhadram <varkabhadram@gmail.com>
>> ---
>>   drivers/net/ieee802154/cc2520.c | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ieee802154/cc2520.c 
>> b/drivers/net/ieee802154/cc2520.c
>> index 15f263c..54c15e0 100644
>> --- a/drivers/net/ieee802154/cc2520.c
>> +++ b/drivers/net/ieee802154/cc2520.c
>> @@ -590,22 +590,23 @@ cc2520_filter(struct ieee802154_hw *hw,
>>             struct ieee802154_hw_addr_filt *filt, unsigned long changed)
>>   {
>>       struct cc2520_private *priv = hw->priv;
>> +    int ret = 0;
>>         if (changed & IEEE802154_AFILT_PANID_CHANGED) {
>>           u16 panid = le16_to_cpu(filt->pan_id);
>>             dev_vdbg(&priv->spi->dev,
>>                "cc2520_filter called for pan id\n");
>> -        cc2520_write_ram(priv, CC2520RAM_PANID,
>> -                 sizeof(panid), (u8 *)&panid);
>> +        ret = cc2520_write_ram(priv, CC2520RAM_PANID,
>> +                       sizeof(panid), (u8 *)&panid);
>>       }
>>         if (changed & IEEE802154_AFILT_IEEEADDR_CHANGED) {
>>           dev_vdbg(&priv->spi->dev,
>>                "cc2520_filter called for IEEE addr\n");
>> -        cc2520_write_ram(priv, CC2520RAM_IEEEADDR,
>> -                 sizeof(filt->ieee_addr),
>> -                 (u8 *)&filt->ieee_addr);
>> +        ret = cc2520_write_ram(priv, CC2520RAM_IEEEADDR,
>> +                       sizeof(filt->ieee_addr),
>> +                       (u8 *)&filt->ieee_addr);
>>       }
>>         if (changed & IEEE802154_AFILT_SADDR_CHANGED) {
>> @@ -613,20 +614,20 @@ cc2520_filter(struct ieee802154_hw *hw,
>>             dev_vdbg(&priv->spi->dev,
>>                "cc2520_filter called for saddr\n");
>> -        cc2520_write_ram(priv, CC2520RAM_SHORTADDR,
>> -                 sizeof(addr), (u8 *)&addr);
>> +        ret = cc2520_write_ram(priv, CC2520RAM_SHORTADDR,
>> +                       sizeof(addr), (u8 *)&addr);
>>       }
>>         if (changed & IEEE802154_AFILT_PANC_CHANGED) {
>>           dev_vdbg(&priv->spi->dev,
>>                "cc2520_filter called for panc change\n");
>>           if (filt->pan_coord)
>> -            cc2520_write_register(priv, CC2520_FRMFILT0, 0x02);
>> +            ret = cc2520_write_register(priv, CC2520_FRMFILT0, 0x02);
>>           else
>> -            cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
>> +            ret = cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
>>       }
>>   -    return 0;
>> +    return ret;
>>   }
>>     static inline int cc2520_set_tx_power(struct cc2520_private 
>> *priv, s32 mbm)
>
> Thanks for the patch.
>
> I found two check patch warnings on this patch.
>

Both fixed.

> As Alex pointed on other thread that you should use commit message
> like "subsytem: file:" or only "subsytem:" in the subject.
> But not "subsytem/subsubsystem:"
>

Well, imho its not a subsubsystem but rather a simple file. Also using 
subsytem: file: looks quite alien to me. Anyway, matter of taste, 
nothing to argue about. Fixed in v2.

regards
Stefan Schmidt
> Thanks.
>