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))
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
