Re: [PATCH] Add dynamic MMC-over-SPI-GPIO driver

[Michael Buesch <mb@bu3sch.de>]

On Monday 14 July 2008 22:54:41 Andrew Morton wrote:
> > +static int gpiommc_probe(struct platform_device *pdev)
> > +{
> > +	static int instance;
> > +	struct gpiommc_device *d = platform_get_drvdata(pdev);
> > +	struct spi_gpio_platform_data pdata;
> > +	int err = -ENOMEM;
> > +
> > +	d->spi_pdev = platform_device_alloc("spi-gpio", instance++);
> > +	if (!d->spi_pdev)
> > +		goto out;
> 
> I guess that incrementing `instance' even if the allocation failed is
> somewhat wrong.

Well, I guess it doesn't matter much. The number is pretty random anyway.

> > +static struct gpiommc_device *gpiommc_alloc(struct platform_device *pdev,
> > +					    const char *name,
> > +					    const struct gpiommc_pins *pins,
> > +					    u8 mode)
> > +{
> > +	struct gpiommc_device *d;
> > +
> > +	d = kmalloc(sizeof(*d), GFP_KERNEL);
> > +	if (!d)
> > +		return NULL;
> > +
> > +	strcpy(d->name, name);
> 
> No check for overruns?

The caller checks the length, but it is a good idea to double-check here.
Good catch.

> > +	memcpy(&d->pins, pins, sizeof(d->pins));
> 
> If this had used the typesafe
> 
> 	d->pins = *pins;
> 
> I wouldn't have needed to run all around the place working out if
> overflow/underflow checks were needed here.

Yeah well, can use this.

> > +static ssize_t gpiommc_add_store(struct device_driver *drv,
> > +				 const char *buf, size_t count)
> > +{
> > +	int res, err;
> > +	char name[GPIOMMC_MAX_NAMELEN + 1];
> > +	struct gpiommc_pins pins;
> > +	unsigned int mode;
> > +
> > +	res = sscanf(buf, "%" GPIOMMC_MAX_NAMELEN_STR "s %u,%u,%u,%u %u",
> > +		     name, &pins.gpio_di, &pins.gpio_do,
> > +		     &pins.gpio_clk, &pins.gpio_cs, &mode);
> 
> What's going on here?  So new kernel/userspace ABI.

The whole point of the module is to create a new userspace interface for
creating the device. The module does just glue several modules together
and create an actual device.

> Not documented in 
> changelog, not documented in code comments, not documented in
> Documentation/ABI.  This forces reviewers to reverse-engineer the
> interface design from the implementation and then attempt to review
> that design.  Reviewers not happy!

Yeah well, as I said, I will do docs later. I didn't have any time
to write documentation, yet.
by. ;)

> > +static ssize_t gpiommc_remove_show(struct device_driver *drv,
> > +				   char *buf)
> > +{
> > +	return snprintf(buf, PAGE_SIZE, "write device-name to remove the device\n");
> > +}
> 
> Now that is one weird way in which to document the interface!  What a
> waste of kernel text :(

Yeah, well. Better than nothing ;)
As I already said in the original patch announcement. This is by no way the
final version of the patch. Docs will be moved to Documentation/

-- 
Greetings Michael.