linux-kernel - RE: [PATCH V1] mfd: da9053: ensure the FAULT

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 8 Aug 2016 11:05:33 +0000
From:	Steve Twiss <stwiss.opensource@...semi.com>
To:	Lee Jones <lee.jones@...aro.org>
CC:	LINUXKERNEL <linux-kernel@...r.kernel.org>,
	Support Opensource <Support.Opensource@...semi.com>
Subject: RE: [PATCH V1] mfd: da9053: ensure the FAULT_LOG is cleared during
 MFD driver probe

On 05 August 2016 10:05, Lee Jones wrote:

> To: Steve Twiss
> Subject: Re: [PATCH V1] mfd: da9053: ensure the FAULT_LOG is cleared during MFD driver probe
> 
> On Wed, 06 Jul 2016, Steve Twiss wrote:
> > From: Steve Twiss <stwiss.opensource@...semi.com>
> >
> > Clearance of any the persistent information within the DA9053 FAULT_LOG
> > register must be completed during start-up so the fault-log does not
> > continue with previous values. A clearance function has been added here in
> > the kernel driver because wiping the fault-log cannot be counted on outside
> > the Linux kernel.

[...]

> > This patch is similar to the requirements for DA9062 and DA9063: to ensure
> > the FAULT_LOG is started from a 'clean' position.
> >
> > The Dialog datasheet for DA9053 suggests: "The FAULT_LOG register has to
> > be cleared from the host after reading, by writing a value of 11111111".
> >
> > See reference:
> >   commit 9011e4a8a6fe57f76511609930ed00d305389089
> >   Author: Steve Twiss <stwiss.opensource@...semi.com>
> >   Date:   Tue May 19 11:32:45 2015 +0100
> 
> Looks like much of the same code.  Can you have a pop at generifying
> this to avoid any unnecessary duplication?

Hi Lee, 

Although the function looks like those found in both the DA9062 and DA9063 drivers, I was just
intending to highlight the "requirement similarities" to clear the fault register on start-up. This
requirement for the Dialog PMIC chips extends across those devices, but the function is not
really general enough to be merged into the kernel core or reused across other Dialog devices.

I don't think there is unnecessary duplication in this particular case for the DA9052/53 driver.

This da9052_clear_fault_log() function will apply to both the Dialog DA9052 and DA9053 PMICs
and so I am making good use of code re-use in this place. But, this does not extend to the
DA9062 or DA9063.

I don't think this FAULT_LOG clearance function in this case is really open to generalisation past
the DA9052/53 driver.

Regards,
Steve

> >  drivers/mfd/da9052-core.c | 51
> +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/mfd/da9052-core.c b/drivers/mfd/da9052-core.c
> > index c0bf68a..a88c206 100644
> > --- a/drivers/mfd/da9052-core.c
> > +++ b/drivers/mfd/da9052-core.c
> > @@ -167,6 +167,7 @@ static bool da9052_reg_writeable(struct device
> *dev, unsigned int reg)
> >  	case DA9052_EVENT_B_REG:
> >  	case DA9052_EVENT_C_REG:
> >  	case DA9052_EVENT_D_REG:
> > +	case DA9052_FAULTLOG_REG:
> >  	case DA9052_IRQ_MASK_A_REG:
> >  	case DA9052_IRQ_MASK_B_REG:
> >  	case DA9052_IRQ_MASK_C_REG:
> > @@ -541,6 +542,52 @@ const struct regmap_config
> da9052_regmap_config = {
> >  };
> >  EXPORT_SYMBOL_GPL(da9052_regmap_config);
> >
> > +static int da9052_clear_fault_log(struct da9052 *da9052)
> > +{
> > +	int ret = 0;
> > +	int fault_log = 0;
> > +
> > +	fault_log = da9052_reg_read(da9052, DA9052_FAULTLOG_REG);
> > +	if (fault_log < 0) {
> > +		dev_err(da9052->dev,
> > +			"Cannot read FAULT_LOG %d\n", fault_log);
> > +		return fault_log;
> > +	}
> > +
> > +	if (fault_log) {
> > +		if (fault_log & DA9052_FAULTLOG_TWDERROR)
> > +			dev_dbg(da9052->dev,
> > +				"Fault log entry detected: TWD_ERROR\n");
> > +		if (fault_log & DA9052_FAULTLOG_VDDFAULT)
> > +			dev_dbg(da9052->dev,
> > +				"Fault log entry detected: VDD_FAULT\n");
> > +		if (fault_log & DA9052_FAULTLOG_VDDSTART)
> > +			dev_dbg(da9052->dev,
> > +				"Fault log entry detected: VDD_START\n");
> > +		if (fault_log & DA9052_FAULTLOG_TEMPOVER)
> > +			dev_dbg(da9052->dev,
> > +				"Fault log entry detected: TEMP_OVER\n");
> > +		if (fault_log & DA9052_FAULTLOG_KEYSHUT)
> > +			dev_dbg(da9052->dev,
> > +				"Fault log entry detected: KEY_SHUT\n");
> > +		if (fault_log & DA9052_FAULTLOG_NSDSET)
> > +			dev_dbg(da9052->dev,
> > +				"Fault log entry detected: nSD_SHUT\n");
> > +		if (fault_log & DA9052_FAULTLOG_WAITSET)
> > +			dev_dbg(da9052->dev,
> > +				"Fault log entry detected: WAIT_SHUT\n");
> > +
> > +		ret = da9052_reg_write(da9052,
> > +					DA9052_FAULTLOG_REG,
> > +					0xFF);
> > +		if (ret < 0)
> > +			dev_err(da9052->dev,
> > +				"Cannot reset FAULT_LOG values %d\n", ret);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  int da9052_device_init(struct da9052 *da9052, u8 chip_id)
> >  {
> >  	struct da9052_pdata *pdata = dev_get_platdata(da9052->dev);
> > @@ -549,6 +596,10 @@ int da9052_device_init(struct da9052 *da9052, u8 chip_id)
> >  	mutex_init(&da9052->auxadc_lock);
> >  	init_completion(&da9052->done);
> >
> > +	ret = da9052_clear_fault_log(da9052);
> > +	if (ret < 0)
> > +		dev_warn(da9052->dev, "Cannot clear FAULT_LOG\n");
> > +
> >  	if (pdata && pdata->init != NULL)
> >  		pdata->init(da9052);
> >