Re: [PATCH] mfd: tps6105x: Fix NULL pointer access.

[Grigoryev Denis]

On Sat., 20/09/2015 Ð 05:20 +0100, Lee Jones wrote:
> On Fri, 04 Sep 2015, Grigoryev Denis wrote:
> 
> > When tps6105x used in TPS6105X_MODE_SHUTDOWN mode the driver calls
> > mfd_add_devices() with mfd_cell->name == NULL, that causes an ooops in
> > platform_device_register() later.
> > 
> > This patch reorders mfd_cell structures and makes code to call
> > mfd_add_devices() with proper number of cells.
> > 
> > Signed-off-by: Denis Grigoryev <grigoryev@xxxxxxxxxxxxxx>
> > ---
> >  drivers/mfd/tps6105x.c |   16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c
> > index 5de95c2..be2c286 100644
> > --- a/drivers/mfd/tps6105x.c
> > +++ b/drivers/mfd/tps6105x.c
> > @@ -124,11 +124,11 @@ static int tps6105x_startup(struct tps6105x *tps6105x)
> >   */
> >  static struct mfd_cell tps6105x_cells[] = {
> >  	{
> > -		/* name will be runtime assigned */
> > +		.name = "tps6105x-gpio",
> >  		.id = -1,
> >  	},
> >  	{
> > -		.name = "tps6105x-gpio",
> > +		/* name will be runtime assigned */
> >  		.id = -1,
> >  	},
> >  };
> 
> So you have 2 cells with identical .name's and identical .id's.
> 
> How does that work?
> 

 No. Here is the first cell with .name = "tps6105x-gpio", the second one
has no name assigned. The name of second cell (or it's absence) will be
chosen in runtime in switch() below depending on selected IC operation
mode.

 This patch is a fix of possible NULL pointer access I encountered with
minimal invasion to the code. I apologize for too short description to
this patch.

> > @@ -138,6 +138,7 @@ static int tps6105x_probe(struct i2c_client *client,
> >  {
> >  	struct tps6105x			*tps6105x;
> >  	struct tps6105x_platform_data	*pdata;
> > +	int n_cells = ARRAY_SIZE(tps6105x_cells);
> >  	int ret;
> >  	int i;
> >  
> > @@ -162,33 +163,34 @@ static int tps6105x_probe(struct i2c_client *client,
> >  	case TPS6105X_MODE_SHUTDOWN:
> >  		dev_info(&client->dev,
> >  			 "present, not used for anything, only GPIO\n");
> > +		n_cells = 1;
> >  		break;
> >  	case TPS6105X_MODE_TORCH:
> > -		tps6105x_cells[0].name = "tps6105x-leds";
> > +		tps6105x_cells[1].name = "tps6105x-leds";
> 
> Now you're over-writing cell names!  I'd say this was a hack.
>

No. Here I assign a name to the second cell. It's been unknown until I
get desired IC mode.

> Please just create an entry for each device, like usual.
> 

Here is only two devices. The first one is "gpio" which always present,
the second one is selected depending on IC mode. A patch with an entry
for each device will be too complex for a fix.

I'm working on "flash" function and DT bindings for this driver so you
can reject this patch if you like.

> >  		dev_warn(&client->dev,
> >  			 "torch mode is unsupported\n");
> >  		break;
> >  	case TPS6105X_MODE_TORCH_FLASH:
> > -		tps6105x_cells[0].name = "tps6105x-flash";
> > +		tps6105x_cells[1].name = "tps6105x-flash";
> >  		dev_warn(&client->dev,
> >  			 "flash mode is unsupported\n");
> >  		break;
> >  	case TPS6105X_MODE_VOLTAGE:
> > -		tps6105x_cells[0].name ="tps6105x-regulator";
> > +		tps6105x_cells[1].name = "tps6105x-regulator";
> >  		break;
> >  	default:
> >  		break;
> >  	}
> >  
> >  	/* Set up and register the platform devices. */
> > -	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
> > +	for (i = 0; i < n_cells; i++) {
> >  		/* One state holder for all drivers, this is simple */
> >  		tps6105x_cells[i].platform_data = tps6105x;
> >  		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
> >  	}
> >  
> >  	return mfd_add_devices(&client->dev, 0, tps6105x_cells,
> > -			       ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL);
> > +			       n_cells, NULL, 0, NULL);
> >  }
> >  
> >  static int tps6105x_remove(struct i2c_client *client)
>