| 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
| ||
|
Message-ID: <20150728110717.GH2492@codeblueprint.co.uk>
Date: Tue, 28 Jul 2015 12:07:17 +0100
From: Matt Fleming <matt@...eblueprint.co.uk>
To: Lee Jones <lee.jones@...aro.org>
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, 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.
> > #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.
> > 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.
> > + 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.
>
> > + 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).
> > + 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.
> > @@ -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?
--
Matt Fleming, Intel Open Source Technology Center
--
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