Re: [PATCH 2/3] i2c: slave-eeprom: add eeprom simulator driver

[Uwe Kleine-König]

On Fri, Nov 21, 2014 at 08:19:41AM +0100, Uwe Kleine-König wrote:
> Hello Wolfram,
> 
> this mail is thematically more a reply to patch 1 and maybe just serves
> my understanding of the slave support.
> 
> On Tue, Nov 18, 2014 at 05:04:54PM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> > 
> > The first user of the i2c-slave interface is an eeprom simulator. It is
> > a shared memory which can be accessed by the remote master via I2C and
> > locally via sysfs.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > 
> > Changes since RFC:
> > * fix build error for modules
> > * don't hardcode size
> > * add boundary checks for sysfs access
> > * use a spinlock
> > * s/at24s/eeprom/g
> > * add some docs
> > * clean up exit paths
> > * use size-variable instead of driver_data
> > 
> >  drivers/i2c/Kconfig            |  10 +++
> >  drivers/i2c/Makefile           |   1 +
> >  drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 181 insertions(+)
> >  create mode 100644 drivers/i2c/i2c-slave-eeprom.c
> > 
> > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> > index b51a402752c4..8c9e619f3026 100644
> > --- a/drivers/i2c/Kconfig
> > +++ b/drivers/i2c/Kconfig
> > @@ -110,6 +110,16 @@ config I2C_STUB
> >  
> >  	  If you don't know what to do here, definitely say N.
> >  
> > +config I2C_SLAVE
> > +	bool "I2C slave support"
> > +
> > +if I2C_SLAVE
> > +
> > +config I2C_SLAVE_EEPROM
> > +	tristate "I2C eeprom slave driver"
> > +
> > +endif
> > +
> >  config I2C_DEBUG_CORE
> >  	bool "I2C Core debugging messages"
> >  	help
> > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> > index 1722f50f2473..45095b3d16a9 100644
> > --- a/drivers/i2c/Makefile
> > +++ b/drivers/i2c/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
> >  obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
> >  obj-y				+= algos/ busses/ muxes/
> >  obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
> > +obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
> >  
> >  ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
> >  CFLAGS_i2c-core.o := -Wno-deprecated-declarations
> > diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> > new file mode 100644
> > index 000000000000..6631400b5f02
> > --- /dev/null
> > +++ b/drivers/i2c/i2c-slave-eeprom.c
> > @@ -0,0 +1,170 @@
> > +/*
> > + * I2C slave mode EEPROM simulator
> > + *
> > + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <wsa@xxxxxxxxxxxxxxxxxxxx>
> > + * Copyright (C) 2014 by Renesas Electronics Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; version 2 of the License.
> > + *
> > + * Because most IP blocks can only detect one I2C slave address anyhow, this
> > + * driver does not support simulating EEPROM types which take more than one
> > + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit
> > + * pointer, yet implementation is deferred until the need actually arises.
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/sysfs.h>
> > +
> > +struct eeprom_data {
> > +	struct bin_attribute bin;
> > +	bool first_write;
> > +	spinlock_t buffer_lock;
> > +	u8 buffer_idx;
> > +	u8 buffer[];
> > +};
> > +
> > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
> > +				     enum i2c_slave_event event, u8 *val)
> > +{
> > +	struct eeprom_data *eeprom = i2c_get_clientdata(client);
> > +
> > +	switch (event) {
> > +	case I2C_SLAVE_REQ_WRITE_END:
> > +		if (eeprom->first_write) {
> > +			eeprom->buffer_idx = *val;
> > +			eeprom->first_write = false;
> > +		} else {
> > +			spin_lock(&eeprom->buffer_lock);
> > +			eeprom->buffer[eeprom->buffer_idx++] = *val;
> > +			spin_unlock(&eeprom->buffer_lock);
> > +		}
> > +		break;
> > +
> > +	case I2C_SLAVE_REQ_READ_START:
> > +		spin_lock(&eeprom->buffer_lock);
> > +		*val = eeprom->buffer[eeprom->buffer_idx];
> > +		spin_unlock(&eeprom->buffer_lock);
> > +		break;
> > +
> > +	case I2C_SLAVE_REQ_READ_END:
> > +		eeprom->buffer_idx++;
> You don't check here for buffer_idx >= ARRAY_SIZE(buffer)?
> Ditto in the I2C_SLAVE_REQ_WRITE_END case.
I just noticed that buffer_idx is an u8, so it overflows at 255+1. So
the probe routine should error out if size is bigger than 256.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/