* [PATCH] mfd: tps6105x: Fix NULL pointer access. @ 2015-09-04 15:14 Grigoryev Denis 2015-09-20 4:20 ` Lee Jones 0 siblings, 1 reply; 5+ messages in thread From: Grigoryev Denis @ 2015-09-04 15:14 UTC (permalink / raw) To: linux-kernel; +Cc: Samuel Ortiz, Lee Jones [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2565 bytes --] 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@spb.prosoft.ru> --- 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, }, }; @@ -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"; 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) -- 1.7.10.4 ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mfd: tps6105x: Fix NULL pointer access. 2015-09-04 15:14 [PATCH] mfd: tps6105x: Fix NULL pointer access Grigoryev Denis @ 2015-09-20 4:20 ` Lee Jones 2015-09-21 11:22 ` Grigoryev Denis 2015-09-21 13:41 ` [PATCH v2] " Grigoryev Denis 0 siblings, 2 replies; 5+ messages in thread From: Lee Jones @ 2015-09-20 4:20 UTC (permalink / raw) To: Grigoryev Denis; +Cc: linux-kernel, Samuel Ortiz 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@spb.prosoft.ru> > --- > 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? > @@ -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. Please just create an entry for each device, like usual. > 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) -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mfd: tps6105x: Fix NULL pointer access. 2015-09-20 4:20 ` Lee Jones @ 2015-09-21 11:22 ` Grigoryev Denis 2015-09-21 13:41 ` [PATCH v2] " Grigoryev Denis 1 sibling, 0 replies; 5+ messages in thread From: Grigoryev Denis @ 2015-09-21 11:22 UTC (permalink / raw) To: Lee Jones; +Cc: linux-kernel, Samuel Ortiz [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3974 bytes --] 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@spb.prosoft.ru> > > --- > > 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) > ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] mfd: tps6105x: Fix NULL pointer access. 2015-09-20 4:20 ` Lee Jones 2015-09-21 11:22 ` Grigoryev Denis @ 2015-09-21 13:41 ` Grigoryev Denis 2015-09-21 17:07 ` Grigoryev Denis 1 sibling, 1 reply; 5+ messages in thread From: Grigoryev Denis @ 2015-09-21 13:41 UTC (permalink / raw) To: Lee Jones; +Cc: linux-kernel, Samuel Ortiz [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3427 bytes --] 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 adds an mfd_cell for each possible mode thereby excluding runtime .name assignment. Signed-off-by: Denis Grigoryev <grigoryev@fastwel.ru> --- drivers/mfd/tps6105x.c | 62 ++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c index 5de95c2..4dbfb45 100644 --- a/drivers/mfd/tps6105x.c +++ b/drivers/mfd/tps6105x.c @@ -119,17 +119,25 @@ static int tps6105x_startup(struct tps6105x *tps6105x) } /* - * MFD cells - we have one cell which is selected operation - * mode, and we always have a GPIO cell. - */ + * * MFD cells - we always have a GPIO cell and we have one cell + * * which is selected operation mode. + * */ static struct mfd_cell tps6105x_cells[] = { - { - /* name will be runtime assigned */ - .id = -1, + [TPS6105X_REG0_MODE_SHUTDOWN] = { + .name = "tps6105x-gpio", + .id = -1, + }, + [TPS6105X_REG0_MODE_TORCH] = { + .name = "tps6105x-leds", + .id = -1, + }, + [TPS6105X_REG0_MODE_TORCH_FLASH] = { + .name = "tps6105x-flash", + .id = -1, }, - { - .name = "tps6105x-gpio", - .id = -1, + [TPS6105X_REG0_MODE_VOLTAGE] = { + .name = "tps6105x-regulator", + .id = -1, }, }; @@ -157,6 +165,18 @@ static int tps6105x_probe(struct i2c_client *client, return ret; } + /* Set up and register the platform devices. */ + for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) { + /* One state holder for all drivers, this is simple */ + tps6105x_cells[i].platform_data = tps6105x; + tps6105x_cells[i].pdata_size = sizeof(*tps6105x); + } + + ret = mfd_add_devices(&client->dev, -1, tps6105x_cells, + 1, NULL, 0, NULL); + if (ret) + return ret; + /* Remove warning texts when you implement new cell drivers */ switch (pdata->mode) { case TPS6105X_MODE_SHUTDOWN: @@ -164,31 +184,21 @@ static int tps6105x_probe(struct i2c_client *client, "present, not used for anything, only GPIO\n"); break; case TPS6105X_MODE_TORCH: - tps6105x_cells[0].name = "tps6105x-leds"; - dev_warn(&client->dev, - "torch mode is unsupported\n"); - break; case TPS6105X_MODE_TORCH_FLASH: - tps6105x_cells[0].name = "tps6105x-flash"; - dev_warn(&client->dev, - "flash mode is unsupported\n"); - break; case TPS6105X_MODE_VOLTAGE: - tps6105x_cells[0].name ="tps6105x-regulator"; + ret = mfd_add_devices(&client->dev, -1, + &tps6105x_cells[pdata->mode], + 1, NULL, 0, NULL); break; default: + dev_warn(&client->dev, "invalid mode: %d\n", pdata->mode); break; } - /* Set up and register the platform devices. */ - for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) { - /* One state holder for all drivers, this is simple */ - tps6105x_cells[i].platform_data = tps6105x; - tps6105x_cells[i].pdata_size = sizeof(*tps6105x); - } + if (ret) + mfd_remove_devices(&client->dev); - return mfd_add_devices(&client->dev, 0, tps6105x_cells, - ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL); + return ret; } static int tps6105x_remove(struct i2c_client *client) -- 1.7.10.4 ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mfd: tps6105x: Fix NULL pointer access. 2015-09-21 13:41 ` [PATCH v2] " Grigoryev Denis @ 2015-09-21 17:07 ` Grigoryev Denis 0 siblings, 0 replies; 5+ messages in thread From: Grigoryev Denis @ 2015-09-21 17:07 UTC (permalink / raw) To: Lee Jones; +Cc: linux-kernel, Samuel Ortiz [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3840 bytes --] Oh, sorry. With further testing I realized that I made a mistake and typos. I will send the patch v3. My apologies for the flood. I'm new in LKML. On Mon., 21/09/2015 at 13:41 +0000, 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 adds an mfd_cell for each possible mode thereby excluding runtime > .name assignment. > > Signed-off-by: Denis Grigoryev <grigoryev@fastwel.ru> > --- > drivers/mfd/tps6105x.c | 62 ++++++++++++++++++++++++++++-------------------- > 1 file changed, 36 insertions(+), 26 deletions(-) > > diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c > index 5de95c2..4dbfb45 100644 > --- a/drivers/mfd/tps6105x.c > +++ b/drivers/mfd/tps6105x.c > @@ -119,17 +119,25 @@ static int tps6105x_startup(struct tps6105x *tps6105x) > } > > /* > - * MFD cells - we have one cell which is selected operation > - * mode, and we always have a GPIO cell. > - */ > + * * MFD cells - we always have a GPIO cell and we have one cell > + * * which is selected operation mode. > + * */ > static struct mfd_cell tps6105x_cells[] = { > - { > - /* name will be runtime assigned */ > - .id = -1, > + [TPS6105X_REG0_MODE_SHUTDOWN] = { > + .name = "tps6105x-gpio", > + .id = -1, > + }, > + [TPS6105X_REG0_MODE_TORCH] = { > + .name = "tps6105x-leds", > + .id = -1, > + }, > + [TPS6105X_REG0_MODE_TORCH_FLASH] = { > + .name = "tps6105x-flash", > + .id = -1, > }, > - { > - .name = "tps6105x-gpio", > - .id = -1, > + [TPS6105X_REG0_MODE_VOLTAGE] = { > + .name = "tps6105x-regulator", > + .id = -1, > }, > }; > > @@ -157,6 +165,18 @@ static int tps6105x_probe(struct i2c_client *client, > return ret; > } > > + /* Set up and register the platform devices. */ > + for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) { > + /* One state holder for all drivers, this is simple */ > + tps6105x_cells[i].platform_data = tps6105x; > + tps6105x_cells[i].pdata_size = sizeof(*tps6105x); > + } > + > + ret = mfd_add_devices(&client->dev, -1, tps6105x_cells, > + 1, NULL, 0, NULL); > + if (ret) > + return ret; > + > /* Remove warning texts when you implement new cell drivers */ > switch (pdata->mode) { > case TPS6105X_MODE_SHUTDOWN: > @@ -164,31 +184,21 @@ static int tps6105x_probe(struct i2c_client *client, > "present, not used for anything, only GPIO\n"); > break; > case TPS6105X_MODE_TORCH: > - tps6105x_cells[0].name = "tps6105x-leds"; > - dev_warn(&client->dev, > - "torch mode is unsupported\n"); > - break; > case TPS6105X_MODE_TORCH_FLASH: > - tps6105x_cells[0].name = "tps6105x-flash"; > - dev_warn(&client->dev, > - "flash mode is unsupported\n"); > - break; > case TPS6105X_MODE_VOLTAGE: > - tps6105x_cells[0].name ="tps6105x-regulator"; > + ret = mfd_add_devices(&client->dev, -1, > + &tps6105x_cells[pdata->mode], > + 1, NULL, 0, NULL); > break; > default: > + dev_warn(&client->dev, "invalid mode: %d\n", pdata->mode); > break; > } > > - /* Set up and register the platform devices. */ > - for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) { > - /* One state holder for all drivers, this is simple */ > - tps6105x_cells[i].platform_data = tps6105x; > - tps6105x_cells[i].pdata_size = sizeof(*tps6105x); > - } > + if (ret) > + mfd_remove_devices(&client->dev); > > - return mfd_add_devices(&client->dev, 0, tps6105x_cells, > - ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL); > + return ret; > } > > static int tps6105x_remove(struct i2c_client *client) ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-09-21 17:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-04 15:14 [PATCH] mfd: tps6105x: Fix NULL pointer access Grigoryev Denis 2015-09-20 4:20 ` Lee Jones 2015-09-21 11:22 ` Grigoryev Denis 2015-09-21 13:41 ` [PATCH v2] " Grigoryev Denis 2015-09-21 17:07 ` Grigoryev Denis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).