We use dev_get_drvdata to get the driver match data associated with a device. This has two shortcomings: - Linux has dev_get_drvdata too, which returns a private pointer for driver specific info to associate with a device. We use dev->priv (or more often container_of) for that in barebox instead - It nearly always involves a cast to a double pointer, which is error-prone as size and alignment match need to be ensured on the programmer's part and can easily be gotten wrong: enum dev_type type; dev_get_drvdata(dev, (const void **)&type); // UB! Add a new function that instead of using a double pointer argument, returns the pointer directly: - For normal pointer driver data, no cast is necessary - For integer driver data casted to a pointer for storage, the cast is still necessary, but it's only a single pointer this way Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- v1 -> v2: - Changed second patch to use pointer instead of enum in match data (Sascha) - Changed third and fourth patch to return error on match with driver name instead of assuming to be compatible with a default type. --- drivers/base/driver.c | 11 +++++++++++ include/driver.h | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/drivers/base/driver.c b/drivers/base/driver.c index 412db6c40699..3205bbc3c33b 100644 --- a/drivers/base/driver.c +++ b/drivers/base/driver.c @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) return -ENODEV; } + +const void *device_get_match_data(struct device_d *dev) +{ + if (dev->of_id_entry) + return dev->of_id_entry->data; + + if (dev->id_entry) + return (void *)dev->id_entry->driver_data; + + return NULL; +} diff --git a/include/driver.h b/include/driver.h index 1961ab6ed9ea..e2886d051d4f 100644 --- a/include/driver.h +++ b/include/driver.h @@ -540,8 +540,26 @@ int devfs_create_partitions(const char *devname, #define DRV_OF_COMPAT(compat) \ IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL +/** + * dev_get_drvdata - get driver match data associated with device + * @dev: device instance + * @data: pointer to void *, where match data is stored + * + * Returns 0 on success and error code otherwise. + * + * DEPRECATED: use device_get_match_data instead, which avoids + * common pitfalls due to explicit pointer casts + */ int dev_get_drvdata(struct device_d *dev, const void **data); +/** + * device_get_match_data - get driver match data associated with device + * @dev: device instance + * + * Returns match data on success and NULL otherwise + */ +const void *device_get_match_data(struct device_d *dev); + int device_match_of_modalias(struct device_d *dev, struct driver_d *drv); #endif /* DRIVER_H */ -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
dev->id_entry is not populated for devices probed from the device tree. It was used unconditionally however. Use device_get_match_data instead to support device tree probing. While at it, remove the array and the enum, we can store pointers to the correct chipdef structs directly. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/led/led-pca955x.c | 77 +++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 40 deletions(-) diff --git a/drivers/led/led-pca955x.c b/drivers/led/led-pca955x.c index 0b8291a6ed49..07bc26a50bec 100644 --- a/drivers/led/led-pca955x.c +++ b/drivers/led/led-pca955x.c @@ -72,53 +72,47 @@ enum led_brightness { LED_FULL = 255, }; -enum pca955x_type { - pca9550, - pca9551, - pca9552, - pca9553, -}; - struct pca955x_chipdef { int bits; u8 slv_addr; /* 7-bit slave address mask */ int slv_addr_shift; /* Number of bits to ignore */ }; -static struct pca955x_chipdef pca955x_chipdefs[] = { - [pca9550] = { - .bits = 2, - .slv_addr = /* 110000x */ 0x60, - .slv_addr_shift = 1, - }, - [pca9551] = { - .bits = 8, - .slv_addr = /* 1100xxx */ 0x60, - .slv_addr_shift = 3, - }, - [pca9552] = { - .bits = 16, - .slv_addr = /* 1100xxx */ 0x60, - .slv_addr_shift = 3, - }, - [pca9553] = { - .bits = 4, - .slv_addr = /* 110001x */ 0x62, - .slv_addr_shift = 1, - }, +static const struct pca955x_chipdef pca9550_chipdef = { + .bits = 2, + .slv_addr = /* 110000x */ 0x60, + .slv_addr_shift = 1, +}; + +static const struct pca955x_chipdef pca9551_chipdef = { + .bits = 8, + .slv_addr = /* 1100xxx */ 0x60, + .slv_addr_shift = 3, +}; + +static const struct pca955x_chipdef pca9552_chipdef = { + .bits = 16, + .slv_addr = /* 1100xxx */ 0x60, + .slv_addr_shift = 3, +}; + +static const struct pca955x_chipdef pca9553_chipdef = { + .bits = 4, + .slv_addr = /* 110001x */ 0x62, + .slv_addr_shift = 1, }; static const struct platform_device_id led_pca955x_id[] = { - { "pca9550", pca9550 }, - { "pca9551", pca9551 }, - { "pca9552", pca9552 }, - { "pca9553", pca9553 }, + { "pca9550", (unsigned long) &pca9550_chipdef }, + { "pca9551", (unsigned long) &pca9551_chipdef }, + { "pca9552", (unsigned long) &pca9552_chipdef }, + { "pca9553", (unsigned long) &pca9553_chipdef }, { } }; struct pca955x { struct pca955x_led *leds; - struct pca955x_chipdef *chipdef; + const struct pca955x_chipdef *chipdef; struct i2c_client *client; }; @@ -278,7 +272,7 @@ static struct pca955x_platform_data * led_pca955x_pdata_of_init(struct device_node *np, struct pca955x *pca955x) { struct device_node *child; - struct pca955x_chipdef *chip = pca955x->chipdef; + const struct pca955x_chipdef *chip = pca955x->chipdef; struct pca955x_platform_data *pdata; int count, err; @@ -334,10 +328,10 @@ led_pca955x_pdata_of_init(struct device_node *np, struct pca955x *pca955x) } static const struct of_device_id of_pca955x_match[] = { - { .compatible = "nxp,pca9550", .data = (void *)pca9550 }, - { .compatible = "nxp,pca9551", .data = (void *)pca9551 }, - { .compatible = "nxp,pca9552", .data = (void *)pca9552 }, - { .compatible = "nxp,pca9553", .data = (void *)pca9553 }, + { .compatible = "nxp,pca9550", .data = &pca9550_chipdef }, + { .compatible = "nxp,pca9551", .data = &pca9551_chipdef }, + { .compatible = "nxp,pca9552", .data = &pca9552_chipdef }, + { .compatible = "nxp,pca9553", .data = &pca9553_chipdef }, {}, }; @@ -345,12 +339,15 @@ static int led_pca955x_probe(struct device_d *dev) { struct pca955x *pca955x; struct pca955x_led *pca955x_led; - struct pca955x_chipdef *chip; + const struct pca955x_chipdef *chip; struct i2c_client *client; int err; struct pca955x_platform_data *pdata; - chip = &pca955x_chipdefs[dev->id_entry->driver_data]; + chip = device_get_match_data(dev); + if (!chip) + return -ENODEV; + client = to_i2c_client(dev); /* Make sure the slave address / chip type combo given is possible */ -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
i.MX8 MM & MN are both ARM64 SoCs with a device compatible with "fsl,imx28-dma-apbh". Probing the barebox device driver on these SoCs would invoke undefined behavior however, because of an errant cast. Fix this. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/dma/apbh_dma.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c index 3bee89f78b85..0e4961f6cbde 100644 --- a/drivers/dma/apbh_dma.c +++ b/drivers/dma/apbh_dma.c @@ -50,6 +50,7 @@ static struct mxs_dma_chan mxs_dma_channels[MXS_MAX_DMA_CHANNELS]; enum mxs_dma_id { + UNKNOWN_DMA_ID, IMX23_DMA, IMX28_DMA, }; @@ -596,9 +597,9 @@ static int apbh_dma_probe(struct device_d *dev) enum mxs_dma_id id; int ret, channel; - ret = dev_get_drvdata(dev, (const void **)&id); - if (ret) - return ret; + id = (enum mxs_dma_id)device_get_match_data(dev); + if (id == UNKNOWN_DMA_ID) + return -ENODEV; apbh_dma = apbh = xzalloc(sizeof(*apbh)); iores = dev_request_mem_resource(dev, 0); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Probing the lm75 device driver on 64 bit systems invokes undefined behavior, because of an errant cast. Fix this. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/aiodev/lm75.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/aiodev/lm75.c b/drivers/aiodev/lm75.c index 8186fd2c2b97..8e5948f46874 100644 --- a/drivers/aiodev/lm75.c +++ b/drivers/aiodev/lm75.c @@ -22,6 +22,7 @@ #define LM75_SHUTDOWN 0x01 enum lm75_type { /* keep sorted in alphabetical order */ + unknown, adt75, ds1775, ds75, @@ -109,9 +110,9 @@ static int lm75_probe(struct device_d *dev) int new, ret; enum lm75_type kind; - ret = dev_get_drvdata(dev, (const void **)&kind); - if (ret) - return ret; + kind = (enum lm75_type)device_get_match_data(dev); + if (kind == unknown) + return -ENODEV; data = xzalloc(sizeof(*data)); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Probing the nand_mxs device driver on 64 bit systems invokes undefined behavior, because of an errant cast. Fix this. No change of behavior for 32-bit SoCs intended. On error, type == 0 == GPMI_MXS as it used to. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/mtd/nand/nand_mxs.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c index 36b6e7ac224e..e2b6e2162076 100644 --- a/drivers/mtd/nand/nand_mxs.c +++ b/drivers/mtd/nand/nand_mxs.c @@ -2145,9 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev) if (mxs_nand_mtd) return -EBUSY; - err = dev_get_drvdata(dev, (const void **)&type); - if (err) - type = GPMI_MXS; + type = (enum gpmi_type)device_get_match_data(dev); nand_info = kzalloc(sizeof(struct mxs_nand_info), GFP_KERNEL); if (!nand_info) { -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
The driver has a couple of issues in how it handles match data: - First use of dev_get_drvdata is superfluous as result is unused - Second use of dev_get_drvdata stores a sizeof(const void *) into an enum typed object - hdmi->dev_type contains a truncated pointer to a struct dw_hdmi_data and compares it with an enum, which will always fail Fix these and while it, refactor the code a bit to get rid of dw_hdmi_data, whose only other member is unused. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/video/imx-ipu-v3/imx-hdmi.c | 39 ++++++----------------------- 1 file changed, 8 insertions(+), 31 deletions(-) diff --git a/drivers/video/imx-ipu-v3/imx-hdmi.c b/drivers/video/imx-ipu-v3/imx-hdmi.c index 17b6e4cc257d..1e55c97d2426 100644 --- a/drivers/video/imx-ipu-v3/imx-hdmi.c +++ b/drivers/video/imx-ipu-v3/imx-hdmi.c @@ -1083,19 +1083,18 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi) /* Workaround to clear the overflow condition */ static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi) { - int count; + int count = 4; u8 val; /* TMDS software reset */ hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ); val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF); - if (hdmi->dev_type == IMX6DL_HDMI) { - hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF); - return; - } - for (count = 0; count < 4; count++) + if (hdmi->dev_type == IMX6DL_HDMI) + count = 1; + + while (count--) hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF); } @@ -1193,28 +1192,13 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) hdmi_writeb(hdmi, ih_mute, HDMI_IH_MUTE); } -struct dw_hdmi_data { - unsigned ipu_mask; - enum dw_hdmi_devtype devtype; -}; - -static struct dw_hdmi_data imx6q_hdmi_data = { - .ipu_mask = 0xf, - .devtype = IMX6Q_HDMI, -}; - -static struct dw_hdmi_data imx6dl_hdmi_data = { - .ipu_mask = 0x3, - .devtype = IMX6DL_HDMI, -}; - static struct of_device_id dw_hdmi_dt_ids[] = { { .compatible = "fsl,imx6q-hdmi", - .data = &imx6q_hdmi_data, + .data = (void *)IMX6Q_HDMI, }, { .compatible = "fsl,imx6dl-hdmi", - .data = &imx6dl_hdmi_data, + .data = (void *)IMX6DL_HDMI, }, { /* sentinel */ } @@ -1276,11 +1260,6 @@ static int dw_hdmi_probe(struct device_d *dev) struct device_node *np = dev->device_node; struct dw_hdmi *hdmi; int ret; - const struct dw_hdmi_data *devtype; - - ret = dev_get_drvdata(dev, (const void **)&devtype); - if (ret) - return ret; hdmi = xzalloc(sizeof(*hdmi)); @@ -1289,9 +1268,7 @@ static int dw_hdmi_probe(struct device_d *dev) hdmi->sample_rate = 48000; hdmi->ratio = 100; - ret = dev_get_drvdata(dev, (const void **)&hdmi->dev_type); - if (ret) - return ret; + hdmi->dev_type = (enum dw_hdmi_devtype)device_get_match_data(dev); hdmi->ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
The dev_get_drvdata instances here all store a valid pointer in the match data and can be readily converted. Do so. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/mfd/da9063.c | 4 +--- drivers/pinctrl/imx-iomux-v3.c | 4 ++-- drivers/pinctrl/pinctrl-at91-pio4.c | 4 ++-- drivers/pinctrl/pinctrl-tegra-xusb.c | 2 +- drivers/pinctrl/pinctrl-tegra30.c | 4 ++-- drivers/serial/serial_ns16550.c | 6 ++---- 6 files changed, 10 insertions(+), 14 deletions(-) diff --git a/drivers/mfd/da9063.c b/drivers/mfd/da9063.c index e48c38affac6..31359cf8b822 100644 --- a/drivers/mfd/da9063.c +++ b/drivers/mfd/da9063.c @@ -370,11 +370,9 @@ static int da9063_probe(struct device_d *dev) { struct da9063 *priv = NULL; struct da906x_device_data const *dev_data; - void const *dev_data_tmp; int ret; - ret = dev_get_drvdata(dev, &dev_data_tmp); - dev_data = ret < 0 ? NULL : dev_data_tmp; + dev_data = device_get_match_data(dev); priv = xzalloc(sizeof(struct da9063)); priv->wd.set_timeout = da9063_watchdog_set_timeout; diff --git a/drivers/pinctrl/imx-iomux-v3.c b/drivers/pinctrl/imx-iomux-v3.c index dc98d674a04b..cec2414a03e2 100644 --- a/drivers/pinctrl/imx-iomux-v3.c +++ b/drivers/pinctrl/imx-iomux-v3.c @@ -168,10 +168,10 @@ static struct pinctrl_ops imx_iomux_v3_ops = { static int imx_pinctrl_dt(struct device_d *dev, void __iomem *base) { struct imx_iomux_v3 *iomux; - struct imx_iomux_v3_data *drvdata = NULL; + const struct imx_iomux_v3_data *drvdata; int ret; - dev_get_drvdata(dev, (const void **)&drvdata); + drvdata = device_get_match_data(dev); iomux = xzalloc(sizeof(*iomux)); iomux->base = base; diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c index 550e953a7081..40bc573e315b 100644 --- a/drivers/pinctrl/pinctrl-at91-pio4.c +++ b/drivers/pinctrl/pinctrl-at91-pio4.c @@ -229,7 +229,7 @@ static struct gpio_ops at91_gpio4_ops = { static int pinctrl_at91_pio4_gpiochip_add(struct device_d *dev, struct pinctrl_at91_pio4 *pinctrl) { - struct at91_pinctrl_data *drvdata; + const struct at91_pinctrl_data *drvdata; struct clk *clk; int ret; @@ -247,7 +247,7 @@ static int pinctrl_at91_pio4_gpiochip_add(struct device_d *dev, return ret; } - dev_get_drvdata(dev, (const void **)&drvdata); + drvdata = device_get_match_data(dev); pinctrl->gpiochip.ops = &at91_gpio4_ops; pinctrl->gpiochip.base = 0; diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c index 6bd884b7e2d9..37d77e5ef663 100644 --- a/drivers/pinctrl/pinctrl-tegra-xusb.c +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c @@ -388,7 +388,7 @@ static int pinctrl_tegra_xusb_probe(struct device_d *dev) dev->priv = padctl; padctl->dev = dev; - dev_get_drvdata(dev, (const void **)&padctl->soc); + padctl->soc = device_get_match_data(dev); iores = dev_request_mem_resource(dev, 0); if (IS_ERR(iores)) { diff --git a/drivers/pinctrl/pinctrl-tegra30.c b/drivers/pinctrl/pinctrl-tegra30.c index 225a65233e7f..e9f35e0c9df4 100644 --- a/drivers/pinctrl/pinctrl-tegra30.c +++ b/drivers/pinctrl/pinctrl-tegra30.c @@ -32,7 +32,7 @@ struct pinctrl_tegra30 { u32 __iomem *mux; } regs; struct pinctrl_device pinctrl; - struct pinctrl_tegra30_drvdata *drvdata; + const struct pinctrl_tegra30_drvdata *drvdata; }; struct tegra_pingroup { @@ -893,7 +893,7 @@ static int pinctrl_tegra30_probe(struct device_d *dev) regs[i] = IOMEM(iores->start); } - dev_get_drvdata(dev, (const void **)&ctrl->drvdata); + ctrl->drvdata = device_get_match_data(dev); ctrl->pinctrl.dev = dev; ctrl->pinctrl.ops = &pinctrl_tegra30_ops; diff --git a/drivers/serial/serial_ns16550.c b/drivers/serial/serial_ns16550.c index f117ab9dc10a..fc6fa7dc3e43 100644 --- a/drivers/serial/serial_ns16550.c +++ b/drivers/serial/serial_ns16550.c @@ -468,12 +468,10 @@ static int ns16550_probe(struct device_d *dev) struct ns16550_priv *priv; struct console_device *cdev; struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data; - struct ns16550_drvdata *devtype; + const struct ns16550_drvdata *devtype; int ret; - ret = dev_get_drvdata(dev, (const void **)&devtype); - if (ret) - devtype = &ns16550_drvdata; + devtype = device_get_match_data(dev) ?: &ns16550_drvdata; priv = xzalloc(sizeof(*priv)); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Wed, Oct 07, 2020 at 11:50:52AM +0200, Ahmad Fatoum wrote: > We use dev_get_drvdata to get the driver match data associated with a > device. This has two shortcomings: > > - Linux has dev_get_drvdata too, which returns a private pointer for > driver specific info to associate with a device. We use dev->priv > (or more often container_of) for that in barebox instead > > - It nearly always involves a cast to a double pointer, which is > error-prone as size and alignment match need to be ensured > on the programmer's part and can easily be gotten wrong: > enum dev_type type; > dev_get_drvdata(dev, (const void **)&type); // UB! > > Add a new function that instead of using a double pointer argument, > returns the pointer directly: > > - For normal pointer driver data, no cast is necessary > - For integer driver data casted to a pointer for storage, > the cast is still necessary, but it's only a single pointer > this way > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> Applied, thanks Sascha > --- > v1 -> v2: > - Changed second patch to use pointer instead of enum in match data > (Sascha) > - Changed third and fourth patch to return error on match with > driver name instead of assuming to be compatible with a default > type. > --- > drivers/base/driver.c | 11 +++++++++++ > include/driver.h | 18 ++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > index 412db6c40699..3205bbc3c33b 100644 > --- a/drivers/base/driver.c > +++ b/drivers/base/driver.c > @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) > > return -ENODEV; > } > + > +const void *device_get_match_data(struct device_d *dev) > +{ > + if (dev->of_id_entry) > + return dev->of_id_entry->data; > + > + if (dev->id_entry) > + return (void *)dev->id_entry->driver_data; > + > + return NULL; > +} > diff --git a/include/driver.h b/include/driver.h > index 1961ab6ed9ea..e2886d051d4f 100644 > --- a/include/driver.h > +++ b/include/driver.h > @@ -540,8 +540,26 @@ int devfs_create_partitions(const char *devname, > #define DRV_OF_COMPAT(compat) \ > IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL > > +/** > + * dev_get_drvdata - get driver match data associated with device > + * @dev: device instance > + * @data: pointer to void *, where match data is stored > + * > + * Returns 0 on success and error code otherwise. > + * > + * DEPRECATED: use device_get_match_data instead, which avoids > + * common pitfalls due to explicit pointer casts > + */ > int dev_get_drvdata(struct device_d *dev, const void **data); > > +/** > + * device_get_match_data - get driver match data associated with device > + * @dev: device instance > + * > + * Returns match data on success and NULL otherwise > + */ > +const void *device_get_match_data(struct device_d *dev); > + > int device_match_of_modalias(struct device_d *dev, struct driver_d *drv); > > #endif /* DRIVER_H */ > -- > 2.28.0 > > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox