Am 30.08.2016 11:46, schrieb Andy Shevchenko:
> On Mon, 2016-08-29 at 20:59 +0000, Kuppuswamy, Sathyanarayanan wrote:
>> Hi Andy/Dan,
>>
>> Thanks for catching this bug. As Andy mentioned, this code is written
>> in this manner to let the get_platform_data() function pointer to
>> return the error value on initialization failure. But it has never
>> been used properly in any of the existing code. So my suggestion is
>> either change the platform_lib code to return ERR_PTR on failure or
>> change the intel_mid_sfi_get_pdata to check for NULL as well. Since
>> all the use case of intel_mid_sfi_get_pdata are void functions, I
>> would prefer to go with second solution. Please let me know your
>> comments.
>>
>> diff --git a/arch/x86/platform/intel-mid/sfi.c
>> b/arch/x86/platform/intel-mid/sfi.c
>> index 051d264..a6bd275 100644
>> --- a/arch/x86/platform/intel-mid/sfi.c
>> +++ b/arch/x86/platform/intel-mid/sfi.c
>> @@ -336,7 +336,7 @@ static void __init sfi_handle_ipc_dev(struct
>> sfi_device_table_entry *pentry,
>> pr_debug("IPC bus, name = %16.16s, irq = 0x%2x\n",
>> pentry->name, pentry->irq);
>> pdata = intel_mid_sfi_get_pdata(dev, pentry);
>> - if (IS_ERR(pdata))
>> + if (IS_ERR_OR_NULL(pdata))
>
> But this looks wrong.
> pdata == NULL is valid case for many devices! In other words pdata is an
> optional argument to the device drivers.
>
Yep, the way is wrong.
NULL can say: get_platform_data does not exists
or get_platform_data() returned NULL (what ever that means).
IMHO it feels better to drop the define and replace it
with a proper function call and error.
#define intel_mid_sfi_get_pdata(dev, priv) \
((dev)->get_platform_data ? (dev)->get_platform_data(priv) : NULL)
void *fkt(struct devs_id *dev, void *info)
{
if ( ! dev->get_platform_data)
return ERR_PTR(-ENOSYS);
return dev->get_platform_data(info);
}
just my 2 cents,
re,
wh
>> return;
>>
>> pdev = platform_device_alloc(pentry->name, 0);
>> @@ -371,7 +371,7 @@ static void __init sfi_handle_spi_dev(struct
>> sfi_device_table_entry *pentry,
>> spi_info.chip_select);
>>
>> pdata = intel_mid_sfi_get_pdata(dev, &spi_info);
>> - if (IS_ERR(pdata))
>> + if (IS_ERR_OR_NULL(pdata))
>> return;
>>
>> spi_info.platform_data = pdata;
>> @@ -398,7 +398,7 @@ static void __init sfi_handle_i2c_dev(struct
>> sfi_device_table_entry *pentry,
>> i2c_info.addr);
>> pdata = intel_mid_sfi_get_pdata(dev, &i2c_info);
>> i2c_info.platform_data = pdata;
>> - if (IS_ERR(pdata))
>> + if (IS_ERR_OR_NULL(pdata))
>> return;
>>
>> if (dev->delay)
>> @@ -424,7 +424,7 @@ static void __init sfi_handle_sd_dev(struct
>> sfi_device_table_entry *pentry,
>> sd_info.max_clk,
>> sd_info.addr);
>> pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
>> - if (IS_ERR(pdata))
>> + if (IS_ERR_OR_NULL(pdata))
>> return;
>>
>> /* Nothing we can do with this for now */
>>
>>
>> Thanks and regards,
>> Sathyanarayanan KN
>>
>> ________________________________________
>> From: Andy Shevchenko [andriy.shevchenko@xxxxxxxxxxxxxxx]
>> Sent: Sunday, August 28, 2016 6:31 AM
>> To: Dan Carpenter
>> Cc: kernel-janitors@xxxxxxxxxxxxxxx; David Cohen; Kuppuswamy,
>> Sathyanarayanan
>> Subject: Re: [bug report] x86/sfi: Enable enumeration of SD devices
>>
>> + David, Sathya
>>
>> On Tue, 2016-08-09 at 20:58 +0300, Dan Carpenter wrote:
>>>
>>> On Tue, Aug 09, 2016 at 06:32:55PM +0300, Andy Shevchenko wrote:
>>>>
>>>>
>>>> On Fri, 2016-07-15 at 22:23 +0300, Dan Carpenter wrote:
>>>>>
>>>>>
>>>>> Hello Andy Shevchenko,
>>>>>
>>>>> The patch 05f310e26fe9: "x86/sfi: Enable enumeration of SD
>>>>> devices"
>>>>> from Jul 12, 2016, leads to the following static checker
>>>>> warning:
>>>>>
>>>>> arch/x86/platform/intel-mid/sfi.c:427 sfi_handle_sd_dev()
>>>>> warn: 'pdata' isn't an ERR_PTR
>>>>>
>>>>> arch/x86/platform/intel-mid/sfi.c
>>>>> 416 memset(&sd_info, 0, sizeof(sd_info));
>>>>> 417 strncpy(sd_info.name, pentry->name,
>>>>> SFI_NAME_LEN);
>>>>> 418 sd_info.bus_num = pentry->host_num;
>>>>> 419 sd_info.max_clk = pentry->max_freq;
>>>>> 420 sd_info.addr = pentry->addr;
>>>>> 421 pr_debug("SD bus = %d, name = %16.16s, max_clk =
>>>>> %d,
>>>>> addr = 0x%x\n",
>>>>> 422 sd_info.bus_num,
>>>>> 423 sd_info.name,
>>>>> 424 sd_info.max_clk,
>>>>> 425 sd_info.addr);
>>>>> 426 pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>> This is a macro calling a function pointer. None of the
>>>>> functions
>>>>> return error pointers. Some return NULL on error but some
>>>>> return
>>>>> NULL
>>>>> on success.
>>>>>
>>>>> 427 if (IS_ERR(pdata))
>>>>> 428 return;
>>>>> 429
>>>>> 430 /* Nothing we can do with this for now */
>>>>> 431 sd_info.platform_data = pdata;
>>>>> 432
>>>>
>>>> Thanks for catching up this. At some point in the future I will
>>>> re-
>>>> check
>>>> all those so called "device lib" files to be aligned to one
>>>> standard. Of
>>>> course you may propose a patch if you feel you can do it.
>>>
>>> I'm a temporary haitus from work but what's the standard supposed
>>> to be?
>>
>> I've checked all upstreamed platform modules (arch/x86/platform/intel-
>> mid/device_libs/) and noticed that not a single one returns ERR_PTR.
>>
>> Though I think the idea was to provide a way to fail initialization in
>> some cases where hardware must be initialized properly. Maybe David or
>> Sathya can shed a light on this.
>>
>> If we decide to change that it should be done for all so called device
>> handlers in sfi.c.
>>
>> --
>> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> Intel Finland Oy
>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
