| Message ID | 20220624155413.399190-1-fabrice.gasnier@foss.st.com |
|---|---|
| Headers | show |
| Series | usb: typec: ucsi: add support for stm32g0 | expand |
Le 24/06/2022 à 17:54, Fabrice Gasnier a écrit : > STM32G0 provides an integrated USB Type-C and power delivery interface. > It can be programmed with a firmware to handle UCSI protocol over I2C > interface. A GPIO is used as an interrupt line. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier-rj0Iel/JR4NBDgjK7y7TUQ@public.gmane.org> > --- > drivers/usb/typec/ucsi/Kconfig | 10 ++ > drivers/usb/typec/ucsi/Makefile | 1 + > drivers/usb/typec/ucsi/ucsi_stm32g0.c | 218 ++++++++++++++++++++++++++ > 3 files changed, 229 insertions(+) > create mode 100644 drivers/usb/typec/ucsi/ucsi_stm32g0.c > [...] > +static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, const void *val, > + size_t len) > +{ > + struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi); > + struct i2c_client *client = g0->client; > + struct i2c_msg msg[] = { > + { > + .addr = client->addr, > + .flags = 0, > + } > + }; > + unsigned char *buf; > + int ret; > + > + buf = kzalloc(len + 1, GFP_KERNEL); Hi, Nit: kmalloc() would be enough here, the whole buffer is written just a few lines after. > + if (!buf) > + return -ENOMEM; > + > + buf[0] = offset; > + memcpy(&buf[1], val, len); > + msg[0].len = len + 1; > + msg[0].buf = buf; > + > + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg)); > + kfree(buf); > + if (ret != ARRAY_SIZE(msg)) { > + dev_err(g0->dev, "i2c write %02x, %02x error: %d\n", client->addr, buf[0], ret); Use-after-free of buf. > + > + return ret < 0 ? ret : -EIO; > + } > + > + return 0; > +} > + Just my 2c, CJ [...]
Hi, On Fri, Jun 24, 2022 at 05:54:11PM +0200, Fabrice Gasnier wrote: > +static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id) > +{ > + struct device *dev = &client->dev; > + struct ucsi_stm32g0 *g0; > + int ret; > + > + g0 = devm_kzalloc(dev, sizeof(*g0), GFP_KERNEL); > + if (!g0) > + return -ENOMEM; > + > + g0->dev = dev; > + g0->client = client; > + init_completion(&g0->complete); > + i2c_set_clientdata(client, g0); > + > + g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops); > + if (IS_ERR(g0->ucsi)) > + return PTR_ERR(g0->ucsi); > + > + ucsi_set_drvdata(g0->ucsi, g0); > + > + /* Request alert interrupt */ > + ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT, > + dev_name(&client->dev), g0); > + if (ret) { > + dev_err_probe(dev, ret, "request IRQ failed\n"); > + goto destroy; > + } > + > + ret = ucsi_register(g0->ucsi); > + if (ret) { > + dev_err_probe(dev, ret, "ucsi_register failed\n"); > + goto freeirq; > + } If there isn't UCSI firmware, then ucsi_register() will always safely fail here, right? > + return 0; > + > +freeirq: > + free_irq(client->irq, g0); > +destroy: > + ucsi_destroy(g0->ucsi); > + > + return ret; > +} thanks,
On 6/27/22 15:17, Heikki Krogerus wrote: > Hi, > > On Fri, Jun 24, 2022 at 05:54:11PM +0200, Fabrice Gasnier wrote: >> +static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id) >> +{ >> + struct device *dev = &client->dev; >> + struct ucsi_stm32g0 *g0; >> + int ret; >> + >> + g0 = devm_kzalloc(dev, sizeof(*g0), GFP_KERNEL); >> + if (!g0) >> + return -ENOMEM; >> + >> + g0->dev = dev; >> + g0->client = client; >> + init_completion(&g0->complete); >> + i2c_set_clientdata(client, g0); >> + >> + g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops); >> + if (IS_ERR(g0->ucsi)) >> + return PTR_ERR(g0->ucsi); >> + >> + ucsi_set_drvdata(g0->ucsi, g0); >> + >> + /* Request alert interrupt */ >> + ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT, >> + dev_name(&client->dev), g0); >> + if (ret) { >> + dev_err_probe(dev, ret, "request IRQ failed\n"); >> + goto destroy; >> + } >> + >> + ret = ucsi_register(g0->ucsi); >> + if (ret) { >> + dev_err_probe(dev, ret, "ucsi_register failed\n"); >> + goto freeirq; >> + } > > If there isn't UCSI firmware, then ucsi_register() will always safely > fail here, right? Hi Heikki, Yes, in such a case, the first i2c read (UCSI_VERSION) in ucsi_register() will return an error and safely fail here. Thanks for reviewing, Best Regards, Fabrice > > >> + return 0; >> + >> +freeirq: >> + free_irq(client->irq, g0); >> +destroy: >> + ucsi_destroy(g0->ucsi); >> + >> + return ret; >> +} > > > thanks, >
On Tue, Jun 28, 2022 at 09:21:12AM +0200, Fabrice Gasnier wrote: > On 6/27/22 15:17, Heikki Krogerus wrote: > > Hi, > > > > On Fri, Jun 24, 2022 at 05:54:11PM +0200, Fabrice Gasnier wrote: > >> +static int ucsi_stm32g0_probe(struct i2c_client *client, const struct i2c_device_id *id) > >> +{ > >> + struct device *dev = &client->dev; > >> + struct ucsi_stm32g0 *g0; > >> + int ret; > >> + > >> + g0 = devm_kzalloc(dev, sizeof(*g0), GFP_KERNEL); > >> + if (!g0) > >> + return -ENOMEM; > >> + > >> + g0->dev = dev; > >> + g0->client = client; > >> + init_completion(&g0->complete); > >> + i2c_set_clientdata(client, g0); > >> + > >> + g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops); > >> + if (IS_ERR(g0->ucsi)) > >> + return PTR_ERR(g0->ucsi); > >> + > >> + ucsi_set_drvdata(g0->ucsi, g0); > >> + > >> + /* Request alert interrupt */ > >> + ret = request_threaded_irq(client->irq, NULL, ucsi_stm32g0_irq_handler, IRQF_ONESHOT, > >> + dev_name(&client->dev), g0); > >> + if (ret) { > >> + dev_err_probe(dev, ret, "request IRQ failed\n"); > >> + goto destroy; > >> + } > >> + > >> + ret = ucsi_register(g0->ucsi); > >> + if (ret) { > >> + dev_err_probe(dev, ret, "ucsi_register failed\n"); > >> + goto freeirq; > >> + } > > > > If there isn't UCSI firmware, then ucsi_register() will always safely > > fail here, right? > > Hi Heikki, > > Yes, in such a case, the first i2c read (UCSI_VERSION) in > ucsi_register() will return an error and safely fail here. Okay, thanks.