| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 28 Jul 2015 12:37:21 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Matt Fleming <matt@...eblueprint.co.uk>
Cc: Wim Van Sebroeck <wim@...ana.be>, linux-kernel@...r.kernel.org,
linux-watchdog@...r.kernel.org,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Jean Delvare <jdelvare@...e.com>,
Wolfram Sang <wsa@...-dreams.de>,
Matt Fleming <matt.fleming@...el.com>,
Peter Tyser <ptyser@...-inc.com>,
Samuel Ortiz <sameo@...ux.intel.com>,
Aaron Sierra <asierra@...-inc.com>
Subject: Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform
data
On Tue, 28 Jul 2015, Matt Fleming wrote:
> On Tue, 28 Jul, at 10:46:43AM, Lee Jones wrote:
> > On Mon, 27 Jul 2015, Matt Fleming wrote:
> >
> > > From: Matt Fleming <matt.fleming@...el.com>
> > >
> > > Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> > > the SMBus, unlike previous generations of PCH/ICH where it was on the
> > > LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> > > a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> > > watchdog driver is kind of backwards anyway.
> > >
> > > This change introduces a new 'struct iTCO_wdt_platform_data' for use
> > > inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> > > code, which neatly avoids having to include lpc_ich headers in the i801
> > > i2c driver.
> > >
> > > A simple translation layer is provided for converting from the existing
> > > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
> > >
> > > Cc: Peter Tyser <ptyser@...-inc.com>
> > > Cc: Samuel Ortiz <sameo@...ux.intel.com>
> > > Cc: Lee Jones <lee.jones@...aro.org>
> > > Cc: Wim Van Sebroeck <wim@...ana.be>
> > > Signed-off-by: Matt Fleming <matt.fleming@...el.com>
> > > ---
> > > drivers/mfd/lpc_ich.c | 32 +++++++++++++++++++++++++++++---
> > > drivers/watchdog/Kconfig | 2 +-
> > > drivers/watchdog/iTCO_wdt.c | 11 +++++------
> > > include/linux/mfd/lpc_ich.h | 6 ------
> > > include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> > > 5 files changed, 53 insertions(+), 16 deletions(-)
> > > create mode 100644 include/linux/platform_data/iTCO_wdt.h
> > >
> > > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > > index 8de34398abc0..d190b74a6321 100644
> > > --- a/drivers/mfd/lpc_ich.c
> > > +++ b/drivers/mfd/lpc_ich.c
> > > @@ -66,6 +66,7 @@
> > > #include <linux/pci.h>
> > > #include <linux/mfd/core.h>
> > > #include <linux/mfd/lpc_ich.h>
> > > +#include <linux/platform_data/iTCO_wdt.h>
> >
> > Lowercase please.
>
> Even though the driver is called iTCO_wdt? It seemed to me to be more
> confusing to start mixing cases rather than sticking with the ugly upper
> case. Especially since when you look in the iTCO_wdt driver all the
> function and type names are written that way.
The driver shouldn't be called that either.
You are the only one. What makes iTCO 'special'?
$ ls drivers/watchdog/ | grep [A-Z]
iTCO_vendor.h
iTCO_vendor_support.c
iTCO_wdt.c
Kconfig
Makefile
Mixed case names (filenames, variables, etc) are frowned upon and
shouldn't be allowed anywhere. Please read Chapter 4 of
Documentation/CodingStyle.
> > > #define ACPIBASE 0x40
> > > #define ACPIBASE_GPE_OFF 0x28
> > > @@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
> > > priv->actrl_pbase_save = reg_save;
> > > }
> > >
> > > -static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
> > > +static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
> > > {
> > > + struct iTCO_wdt_platform_data *pdata;
> >
> > Lowercase please.
>
> See above.
Likewise. ;)
> > > struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > > + struct lpc_ich_info *info;
> > > + struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> > > +
> > > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > + if (!pdata)
> > > + return -ENOMEM;
> >
> > Where is this freed?
> >
> > Better to use devm_*
>
> Yeah, Guenter caught this too. devm_* would definitely be better.
Great.
> > > + info = &lpc_chipset_info[priv->chipset];
> > > +
> > > + pdata->iTCO_version = info->iTCO_version;
> >
> > Lowercase please.
>
> Hmm... but then this line will read,
>
> pdata->itco_version = info->iTCO_version;
>
> I'm not sure that's an improvement.
Please consider making all of the variable names conform to the
coding standards we normally abide by. You can submit it either as
patch 1 of this set, or independently.
> > > + strcpy(pdata->name, info->name);
> >
> > strncpy() is safer.
>
> OK, I'll update this. Though it's worth pointing out that the name[]
> declarations are of identical size in these two objects (but I guess
> that could change in the future).
Better to err on the side of caution.
> > > + cell->platform_data = pdata;
> > > + cell->pdata_size = sizeof(*pdata);
> > > + return 0;
> > > +}
> > > +
> > > +static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
> > > +{
> > > + struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > > + struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
> > >
> > > cell->platform_data = &lpc_chipset_info[priv->chipset];
> > > cell->pdata_size = sizeof(struct lpc_ich_info);
> >
> > It's pretty hard to tell from the patch without applying it, but what
> > are the actual similarities and differences between the two finalise
> > functions? They looks like they share enough lines for it to make
> > sense to have one function call and do different things in say a
> > switch statement, no?
>
> For LPC_WDT we dynamically allocate the platform data, and for LPC_GPIO
> we use the static lpc_chipsec_info array.
>
> I'm just personally not a fan of performing memory allocations from
> within switch statement bodies, which is why I implemented this as two
> separate finalize functions.
I'll assume this is okay, then take a look at the driver as a whole
once it's applied.
> > > @@ -933,7 +956,7 @@ gpe0_done:
> > > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > > lpc_ich_enable_gpio_space(dev);
> > >
> > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > > + lpc_ich_finalize_gpio_cell(dev);
> > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> > >
> > > @@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > > res->end = base_addr + ACPIBASE_PMC_END;
> > > }
> > >
> > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > > + ret = lpc_ich_finalize_wdt_cell(dev);
> > > + if (ret)
> > > + goto wdt_done;
> > > +
> > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> >
> > Why do you have an mfd_add_devices() call for each device?
>
> Good question. This call has been present since March 2012 when support
> was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> iTCO_wdt driver to mfd model").
>
> There's no good reason that I can see. Aaron?
Thanks for checking.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists