Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Gabor Juhos <juhosg@openwrt.org>]

2012.12.19. 0:58 keltezéssel, Julian Calaby írta:
> Hi Gabor,
> 
> One minor question:
> 
> On Wed, Dec 19, 2012 at 3:22 AM, Gabor Juhos <juhosg@openwrt.org> wrote:
>> An ioremap call is allowed to fail, however
>> the return value of that is not checked in
>> the 'rt2800pci_read_eeprom_soc' function.
>>
>> The patch adds the missing check, and makes
>> the function return an int value. The patch
>> also converts the 'rt2800_read_eeprom' and
>> 'rt2800_ops.read_eeprom' functions to return
>> an int value, so the error value can be
>> propagated up to the 'rt2800_validate_eeprom'
>> function.
>>
>> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
>> ---
>>  drivers/net/wireless/rt2x00/rt2800lib.c |    5 ++++-
>>  drivers/net/wireless/rt2x00/rt2800lib.h |    6 +++---
>>  drivers/net/wireless/rt2x00/rt2800pci.c |   17 ++++++++++++-----
>>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +++-
>>  4 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
>> index 9224d87..5fc16dd 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
>> @@ -970,14 +974,17 @@ static irqreturn_t rt2800pci_interrupt(int irq, void *dev_instance)
>>  /*
>>   * Device probe functions.
>>   */
>> -static void rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> +static int rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>>  {
>>         if (rt2x00_is_soc(rt2x00dev))
>> -               rt2800pci_read_eeprom_soc(rt2x00dev);
>> -       else if (rt2800pci_efuse_detect(rt2x00dev))
>> +               return rt2800pci_read_eeprom_soc(rt2x00dev);
>> +
>> +       if (rt2800pci_efuse_detect(rt2x00dev))
>>                 rt2800pci_read_eeprom_efuse(rt2x00dev);
>>         else
>>                 rt2800pci_read_eeprom_pci(rt2x00dev);
> 
> Would it make any sense to have rt2800pci_read_eeprom_efuse() and
> rt2800pci_read_eeprom_pci() return a value, simply for consistency
> with rt2800pci_read_eeprom_soc()?

I wanted to keep the change as minimal as possible, but this would make sense.

> 
>> +
>> +       return 0;
>>  }
>>
>>  static const struct ieee80211_ops rt2800pci_mac80211_ops = {
>> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
>> index 5c149b5..48de5c9 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
>> @@ -735,13 +735,15 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>>  /*
>>   * Device probe functions.
>>   */
>> -static void rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> +static int rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>>  {
>>         if (rt2800_efuse_detect(rt2x00dev))
>>                 rt2800_read_eeprom_efuse(rt2x00dev);
>>         else
>>                 rt2x00usb_eeprom_read(rt2x00dev, rt2x00dev->eeprom,
>>                                       EEPROM_SIZE);
> 
> Same here.

Yes, especially that the 'rt2x00usb_eeprom_read' function returns an int value
already.

Thank you for the review!

-Gabor