[PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[Guenter Roeck]

From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>

The parameter passed to pca9665_reset is adap->data, not adap. Unless
adap->data happens to point back to adap, this can result in a kernel panic.

Fix re-factoring pca_reset() from a macro to a function to handle chip
specific code, and only call adap->reset_chip() if the chip is not PCA9665.

Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-pca.c |   27 ++++++++++++++-------------
 include/linux/i2c-algo-pca.h     |    1 +
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
index 73133b1..07ccbef 100644
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -46,14 +46,19 @@ static int i2c_debug;
 #define pca_set_con(adap, val) pca_outw(adap, I2C_PCA_CON, val)
 #define pca_get_con(adap) pca_inw(adap, I2C_PCA_CON)
 #define pca_wait(adap) adap->wait_for_completion(adap->data)
-#define pca_reset(adap) adap->reset_chip(adap->data)
 
-static void pca9665_reset(void *pd)
+static void pca_reset(struct i2c_algo_pca_data *adap)
 {
-	struct i2c_algo_pca_data *adap = pd;
-	pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET);
-	pca_outw(adap, I2C_PCA_IND, 0xA5);
-	pca_outw(adap, I2C_PCA_IND, 0x5A);
+	if (adap->chip == I2C_PCA_CHIP_9665) {
+		/* Ignore the reset function from the module,
+		 * we can use the parallel bus reset.
+		 */
+		pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET);
+		pca_outw(adap, I2C_PCA_IND, 0xA5);
+		pca_outw(adap, I2C_PCA_IND, 0x5A);
+	} else {
+		adap->reset_chip(adap->data);
+	}
 }
 
 /*
@@ -378,11 +383,12 @@ static unsigned int pca_probe_chip(struct i2c_adapter *adap)
 	pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_IADR);
 	if (pca_inw(pca_data, I2C_PCA_IND) == 0xAA) {
 		printk(KERN_INFO "%s: PCA9665 detected.\n", adap->name);
-		return I2C_PCA_CHIP_9665;
+		pca_data->chip = I2C_PCA_CHIP_9665;
 	} else {
 		printk(KERN_INFO "%s: PCA9564 detected.\n", adap->name);
-		return I2C_PCA_CHIP_9564;
+		pca_data->chip = I2C_PCA_CHIP_9564;
 	}
+	return pca_data->chip;
 }
 
 static int pca_init(struct i2c_adapter *adap)
@@ -456,11 +462,6 @@ static int pca_init(struct i2c_adapter *adap)
 		 */
 		int raise_fall_time;
 
-		/* Ignore the reset function from the module,
-		 * we can use the parallel bus reset
-		 */
-		pca_data->reset_chip = pca9665_reset;
-
 		if (pca_data->i2c_clock > 1265800) {
 			printk(KERN_WARNING "%s: I2C clock speed too high."
 				" Using 1265.8kHz.\n", adap->name);
diff --git a/include/linux/i2c-algo-pca.h b/include/linux/i2c-algo-pca.h
index 1364d62..a3c3ecd 100644
--- a/include/linux/i2c-algo-pca.h
+++ b/include/linux/i2c-algo-pca.h
@@ -62,6 +62,7 @@ struct i2c_algo_pca_data {
 	 * 330000, 288000, 217000, 146000, 88000, 59000, 44000, 36000
 	 * For PCA9665, use the frequency you want here. */
 	unsigned int			i2c_clock;
+	unsigned int			chip;
 };
 
 int i2c_pca_add_bus(struct i2c_adapter *);
-- 
1.7.9.7

[PATCH 2/2] i2c: (algo-pca) Fix mode selection for PCA9665

[Guenter Roeck]

From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>

As implemented, the code always selects turbo mode for PCA9665, no matter which
clock frequency is configured. This is because it compares clock boundaries
against constants reflecting (boundary / 100), but uses the actual clock frequency
to compare against. To fix the problem, use a variable reflecting (clock / 100),
not the actual clock frequency, when comparing against (boundary / 100).

Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-pca.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
index 07ccbef..572251b 100644
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -477,17 +477,17 @@ static int pca_init(struct i2c_adapter *adap)
 		/* To avoid integer overflow, use clock/100 for calculations */
 		clock = pca_clock(pca_data) / 100;
 
-		if (pca_data->i2c_clock > 10000) {
+		if (clock > 10000) {
 			mode = I2C_PCA_MODE_TURBO;
 			min_tlow = 14;
 			min_thi  = 5;
 			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
-		} else if (pca_data->i2c_clock > 4000) {
+		} else if (clock > 4000) {
 			mode = I2C_PCA_MODE_FASTP;
 			min_tlow = 17;
 			min_thi  = 9;
 			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
-		} else if (pca_data->i2c_clock > 1000) {
+		} else if (clock > 1000) {
 			mode = I2C_PCA_MODE_FAST;
 			min_tlow = 44;
 			min_thi  = 20;
-- 
1.7.9.7

Re: [PATCH 2/2] i2c: (algo-pca) Fix mode selection for PCA9665

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

On Wed, Sep 12, 2012 at 08:39:51PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> 
> As implemented, the code always selects turbo mode for PCA9665, no matter which
> clock frequency is configured. This is because it compares clock boundaries
> against constants reflecting (boundary / 100), but uses the actual clock frequency
> to compare against. To fix the problem, use a variable reflecting (clock / 100),
> not the actual clock frequency, when comparing against (boundary / 100).

Good point, however...

> 
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-pca.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
> index 07ccbef..572251b 100644
> --- a/drivers/i2c/algos/i2c-algo-pca.c
> +++ b/drivers/i2c/algos/i2c-algo-pca.c
> @@ -477,17 +477,17 @@ static int pca_init(struct i2c_adapter *adap)
>  		/* To avoid integer overflow, use clock/100 for calculations */
>  		clock = pca_clock(pca_data) / 100;
>  
> -		if (pca_data->i2c_clock > 10000) {
> +		if (clock > 10000) {

... we should make the code more readable if we are at it.
I'd suggest to drop the "/ 100" from above and move it below this
if-block. This will allow for much more readable comparisons, e.g.
"clock > 1000000" etc which is really the clock in Hz.

>  			mode = I2C_PCA_MODE_TURBO;
>  			min_tlow = 14;
>  			min_thi  = 5;
>  			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> -		} else if (pca_data->i2c_clock > 4000) {
> +		} else if (clock > 4000) {
>  			mode = I2C_PCA_MODE_FASTP;
>  			min_tlow = 17;
>  			min_thi  = 9;
>  			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> -		} else if (pca_data->i2c_clock > 1000) {
> +		} else if (clock > 1000) {
>  			mode = I2C_PCA_MODE_FAST;
>  			min_tlow = 44;
>  			min_thi  = 20;
> -- 
> 1.7.9.7
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2] i2c: (algo-pca) Fix mode selection for PCA9665

[Guenter Roeck]

On Thu, Sep 13, 2012 at 12:05:31PM +0200, Wolfram Sang wrote:
> On Wed, Sep 12, 2012 at 08:39:51PM -0700, Guenter Roeck wrote:
> > From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> > 
> > As implemented, the code always selects turbo mode for PCA9665, no matter which
> > clock frequency is configured. This is because it compares clock boundaries
> > against constants reflecting (boundary / 100), but uses the actual clock frequency
> > to compare against. To fix the problem, use a variable reflecting (clock / 100),
> > not the actual clock frequency, when comparing against (boundary / 100).
> 
> Good point, however...
> 
> > 
> > Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> > Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> > ---
> >  drivers/i2c/algos/i2c-algo-pca.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
> > index 07ccbef..572251b 100644
> > --- a/drivers/i2c/algos/i2c-algo-pca.c
> > +++ b/drivers/i2c/algos/i2c-algo-pca.c
> > @@ -477,17 +477,17 @@ static int pca_init(struct i2c_adapter *adap)
> >  		/* To avoid integer overflow, use clock/100 for calculations */
> >  		clock = pca_clock(pca_data) / 100;
> >  
> > -		if (pca_data->i2c_clock > 10000) {
> > +		if (clock > 10000) {
> 
> ... we should make the code more readable if we are at it.
> I'd suggest to drop the "/ 100" from above and move it below this
> if-block. This will allow for much more readable comparisons, e.g.
> "clock > 1000000" etc which is really the clock in Hz.
> 
We can simply compare against the real frequency:
		if (pca_data->i2c_clock > 1000000) {
which also makes the description a bit easier to understand.

> >  			mode = I2C_PCA_MODE_TURBO;
> >  			min_tlow = 14;
> >  			min_thi  = 5;
> >  			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> > -		} else if (pca_data->i2c_clock > 4000) {
> > +		} else if (clock > 4000) {
> >  			mode = I2C_PCA_MODE_FASTP;
> >  			min_tlow = 17;
> >  			min_thi  = 9;
> >  			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> > -		} else if (pca_data->i2c_clock > 1000) {
> > +		} else if (clock > 1000) {
> >  			mode = I2C_PCA_MODE_FAST;
> >  			min_tlow = 44;
> >  			min_thi  = 20;
> > -- 
> > 1.7.9.7
> > 
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> 
> The parameter passed to pca9665_reset is adap->data, not adap. Unless
> adap->data happens to point back to adap, this can result in a kernel panic.

Like every write and read to a register which uses the same assumption
AFAICS?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[Guenter Roeck]

On Thu, Sep 13, 2012 at 12:14:23PM +0200, Wolfram Sang wrote:
> On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> > From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> > 
> > The parameter passed to pca9665_reset is adap->data, not adap. Unless
> > adap->data happens to point back to adap, this can result in a kernel panic.
> 
> Like every write and read to a register which uses the same assumption
> AFAICS?
> 
You lost me there. Other reset functions are aware of and use the passed parameter
(adap->data). pca9665_reset overwrites the original reset function with
	pca_data->reset_chip = pca9665_reset;
but not ->data (which it can't overwrite since it is used by the read_byte and
write_byte functions). 

	static void pca9665_reset(void *pd)
		struct i2c_algo_pca_data *adap = pd;
	static void i2c_pca_pf_resetchip(void *pd)
		struct i2c_pca_pf_data *i2c = pd;
	static int i2c_pca_pf_readbyte32(void *pd, int reg)
		struct i2c_pca_pf_data *i2c = pd;

Guenter

Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[Thomas Kavanagh]

Wolfram,

  Hopefully the information below explains the problem with the current pca9665_reset() function.

Regards,

Tom

Let's say mod1 has the following

typedef struct mod1_pca_data_ {
    int                   number;
    unsigned int          bus;
    char                  name[50];
    i2c_algo_pca_data    *adap;
} mod1_pca_data_t;

static struct i2c_algo_pca_data mod1_pca_algo_data = {
    .write_byte             = mod1_writebyte,
    .read_byte              = mod1_readbyte,
    .wait_for_completion    = mod1_pca_waitforcompletion,
    .reset_chip             = mod1_pca_resetchip,
    .i2c_clock              = 400000,
};

/*
 *
 */
static struct i2c_adapter mod1_pca_adapter = {
    .owner          = THIS_MODULE,
    .class          = I2C_CLASS_HWMON,
    .algo_data      = &mod1_pca_algo_data,
    .name           = "Mod1 PCA Bus Adapter",
    .timeout        = HZ,
};


void mod1_pca_attach(struct device *dev, mod1_data_t *mod1_data)
{
    mod1_pca_algo_data.data = mod1_data;

    mod1_data->adap = &mod1_pca_adapter;

    mod1_data->adap->dev.parent  = dev;

    i2c_pca_add_bus(mod1_data->adap);
}


Within the i2c-algo-pca module, all calls to pca_outw(adap) and pca_inw(adap) will be done with an input parameter of type i2c_algo_pca_data.  These will intern call mod1's read_byte and write byte functions and pass in adap->data as the first parameter.  This data pointer is of type mod1_pca_data_t * in this example.

Now with the pca9665_reset() function, the i2c-algo-pca module sets the mod1's reset_chip function pointer to pca9665_reset.  When pca_reset(adap) is called, the input parameter is of type i2c_algo_pca_data as expected.  Then pca_reset() calls adap->reset_chip (pca9665_reset) with the first parameter being adap->data.  This parameter is of type mod1_pca_data_t * in this example.  The problem comes from the fact that pca9665_reset(void *pd) is expecting the parameter to be of type i2c_algp_pca_data:

static void pca9665_reset(void *pd)
{
    struct i2c_algo_pca_data *adap = pd;
    pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET);
    pca_outw(adap, I2C_PCA_IND, 0xA5);
    pca_outw(adap, I2C_PCA_IND, 0x5A);
}

All calls to pca_outw(adap) in this function are passing in a pointer to mod1_pca_data_t and not i2c_algo_pca_data which in turn causes the kernel to panic when the pca_outw(adap) macro tries to access adap->write_byte().




On 9/13/12 6:41 AM, "Guenter Roeck" <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org<mailto:linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>> wrote:

On Thu, Sep 13, 2012 at 12:14:23PM +0200, Wolfram Sang wrote:
On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org<mailto:tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>>
>
> The parameter passed to pca9665_reset is adap->data, not adap. Unless
> adap->data happens to point back to adap, this can result in a kernel panic.
Like every write and read to a register which uses the same assumption
AFAICS?
You lost me there. Other reset functions are aware of and use the passed parameter
(adap->data). pca9665_reset overwrites the original reset function with
pca_data->reset_chip = pca9665_reset;
but not ->data (which it can't overwrite since it is used by the read_byte and
write_byte functions).

static void pca9665_reset(void *pd)
struct i2c_algo_pca_data *adap = pd;
static void i2c_pca_pf_resetchip(void *pd)
struct i2c_pca_pf_data *i2c = pd;
static int i2c_pca_pf_readbyte32(void *pd, int reg)
struct i2c_pca_pf_data *i2c = pd;

Guenter

Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]

Thomas,

>   Hopefully the information below explains the problem with the current pca9665_reset() function.

Yes, it does :)

> Let's say mod1 has the following

This was the first needed information. You seem to not use
i2c-pca-platform but a custom driver using the pca-algo, right? May I
ask why the custom one is needed?

> Now with the pca9665_reset() function, the i2c-algo-pca module sets
> the mod1's reset_chip function pointer to pca9665_reset.  When
> pca_reset(adap) is called, the input parameter is of type
> i2c_algo_pca_data as expected.  Then pca_reset() calls
> adap->reset_chip (pca9665_reset) with the first parameter being
> adap->data.  This parameter is of type mod1_pca_data_t * in this
> example.  The problem comes from the fact that pca9665_reset(void *pd)
> is expecting the parameter to be of type i2c_algp_pca_data:

To put in other words: All other wrappers from the algo call back to the
bus driver which knows to handle its custom data. Only the pca9665 reset
is staying inside the algo and facing a custom data structure it
doesn't know. Okay, understood now.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[Thomas Kavanagh]

On 9/14/12 5:12 AM, "Wolfram Sang" <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

>Thomas,
>
>>   Hopefully the information below explains the problem with the current
>>pca9665_reset() function.
>
>Yes, it does :)
>
>> Let's say mod1 has the following
>
>This was the first needed information. You seem to not use
>i2c-pca-platform but a custom driver using the pca-algo, right? May I
>ask why the custom one is needed?

A custom one is required as we have another device that we must go through
in order to access the PCA9665 controller.

>
>> Now with the pca9665_reset() function, the i2c-algo-pca module sets
>> the mod1's reset_chip function pointer to pca9665_reset.  When
>> pca_reset(adap) is called, the input parameter is of type
>> i2c_algo_pca_data as expected.  Then pca_reset() calls
>> adap->reset_chip (pca9665_reset) with the first parameter being
>> adap->data.  This parameter is of type mod1_pca_data_t * in this
>> example.  The problem comes from the fact that pca9665_reset(void *pd)
>> is expecting the parameter to be of type i2c_algp_pca_data:
>
>To put in other words: All other wrappers from the algo call back to the
>bus driver which knows to handle its custom data. Only the pca9665 reset
>is staying inside the algo and facing a custom data structure it
>doesn't know. Okay, understood now.
>
>Thanks,
>
>   Wolfram
>
>-- 
>Pengutronix e.K.                           | Wolfram Sang                |
>Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> 
> The parameter passed to pca9665_reset is adap->data, not adap. Unless
> adap->data happens to point back to adap, this can result in a kernel panic.

Can you please update this paragraph with a short explanation of what we
discussed (i.e. why exactly the kernel can crash)?

> 
> Fix re-factoring pca_reset() from a macro to a function to handle chip
> specific code, and only call adap->reset_chip() if the chip is not PCA9665.
> 
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>

Patch itself is good.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]