* [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
@ 2020-09-28 15:42 Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 2/7] led: pca955x: fix probing from device tree Ahmad Fatoum
` (8 more replies)
0 siblings, 9 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:42 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
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);
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
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
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 154525e0fde7..14220431f2d6 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -534,8 +534,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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/7] led: pca955x: fix probing from device tree
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
@ 2020-09-28 15:42 ` Ahmad Fatoum
2020-09-29 7:19 ` Sascha Hauer
2020-09-28 15:42 ` [PATCH 3/7] dma: apbh: fix out-of-bounds write on 64-bit SoCs Ahmad Fatoum
` (7 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:42 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
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.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/led/led-pca955x.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/led/led-pca955x.c b/drivers/led/led-pca955x.c
index 27fefce8d524..f89fcbfba5ac 100644
--- a/drivers/led/led-pca955x.c
+++ b/drivers/led/led-pca955x.c
@@ -349,8 +349,11 @@ static int led_pca955x_probe(struct device_d *dev)
struct i2c_client *client;
int err;
struct pca955x_platform_data *pdata;
+ enum pca955x_type type;
- chip = &pca955x_chipdefs[dev->id_entry->driver_data];
+ type = (enum pca955x_type)device_get_match_data(dev);
+
+ chip = &pca955x_chipdefs[type];
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/7] dma: apbh: fix out-of-bounds write on 64-bit SoCs
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 2/7] led: pca955x: fix probing from device tree Ahmad Fatoum
@ 2020-09-28 15:42 ` Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 4/7] aiodev: lm75: " Ahmad Fatoum
` (6 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:42 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
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.
This has a side-effect: Whereas before, probing devices matched by
driver name failed with -ENODEV, they are now considered to be
compatible to IMX23_DMA instead. As we don't depend on this, this
is deemed acceptable.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/dma/apbh_dma.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c
index 3bee89f78b85..186c69388c7f 100644
--- a/drivers/dma/apbh_dma.c
+++ b/drivers/dma/apbh_dma.c
@@ -596,9 +596,7 @@ 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);
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/7] aiodev: lm75: fix out-of-bounds write on 64-bit SoCs
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 2/7] led: pca955x: fix probing from device tree Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 3/7] dma: apbh: fix out-of-bounds write on 64-bit SoCs Ahmad Fatoum
@ 2020-09-28 15:42 ` Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 5/7] mtd: nand-mxs: " Ahmad Fatoum
` (5 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:42 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Probing the lm75 device driver on 64 bit systems invokes
undefined behavior, because of an errant cast. Fix this.
This has a side-effect: Whereas before, probing devices matched by
driver name failed with -ENODEV, they are now considered to be
compatible to adt75 instead. As we don't depend on this, this
is deemed acceptable.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/aiodev/lm75.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/aiodev/lm75.c b/drivers/aiodev/lm75.c
index 8186fd2c2b97..4bdccaad5828 100644
--- a/drivers/aiodev/lm75.c
+++ b/drivers/aiodev/lm75.c
@@ -109,9 +109,7 @@ 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);
data = xzalloc(sizeof(*data));
--
2.28.0
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/7] mtd: nand-mxs: fix out-of-bounds write on 64-bit SoCs
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
` (2 preceding siblings ...)
2020-09-28 15:42 ` [PATCH 4/7] aiodev: lm75: " Ahmad Fatoum
@ 2020-09-28 15:42 ` Ahmad Fatoum
2020-09-28 16:01 ` Marco Felsch
2020-09-28 15:42 ` [PATCH 6/7] video: imx-hdmi: fix dev_get_drvdata misuse Ahmad Fatoum
` (4 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:42 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
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.
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/7] video: imx-hdmi: fix dev_get_drvdata misuse
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
` (3 preceding siblings ...)
2020-09-28 15:42 ` [PATCH 5/7] mtd: nand-mxs: " Ahmad Fatoum
@ 2020-09-28 15:42 ` Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 7/7] driver: migrate some from dev_get_drvdata to device_get_match_data Ahmad Fatoum
` (3 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:42 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 7/7] driver: migrate some from dev_get_drvdata to device_get_match_data
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
` (4 preceding siblings ...)
2020-09-28 15:42 ` [PATCH 6/7] video: imx-hdmi: fix dev_get_drvdata misuse Ahmad Fatoum
@ 2020-09-28 15:42 ` Ahmad Fatoum
2020-09-28 15:46 ` [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
` (2 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:42 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
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 fd0527451276..4276981d94c0 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 b527114f1ba0..18b1aded6a1b 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 c4d3bbe8d429..558742665ea8 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 278ea8c4a0b3..cc71b79f6c9b 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
` (5 preceding siblings ...)
2020-09-28 15:42 ` [PATCH 7/7] driver: migrate some from dev_get_drvdata to device_get_match_data Ahmad Fatoum
@ 2020-09-28 15:46 ` Ahmad Fatoum
2020-09-29 6:45 ` Sascha Hauer
2020-09-29 7:32 ` Sascha Hauer
8 siblings, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 15:46 UTC (permalink / raw)
To: Ahmad Fatoum, barebox
On 9/28/20 5:42 PM, 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);
>
> 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
That ", but" should be removed. Sascha, could you fix this up
on apply (assuming there won't be a v2)?
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> 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 154525e0fde7..14220431f2d6 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -534,8 +534,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 */
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] mtd: nand-mxs: fix out-of-bounds write on 64-bit SoCs
2020-09-28 15:42 ` [PATCH 5/7] mtd: nand-mxs: " Ahmad Fatoum
@ 2020-09-28 16:01 ` Marco Felsch
2020-09-28 16:03 ` Ahmad Fatoum
0 siblings, 1 reply; 20+ messages in thread
From: Marco Felsch @ 2020-09-28 16:01 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 20-09-28 17:42, Ahmad Fatoum wrote:
> 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.
>
> 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);
Should we add:
if (!type)
type = GPMI_MXS;
here?
> 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
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] mtd: nand-mxs: fix out-of-bounds write on 64-bit SoCs
2020-09-28 16:01 ` Marco Felsch
@ 2020-09-28 16:03 ` Ahmad Fatoum
2020-09-28 16:08 ` Marco Felsch
0 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-28 16:03 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 9/28/20 6:01 PM, Marco Felsch wrote:
> On 20-09-28 17:42, Ahmad Fatoum wrote:
>> 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.
>>
>> 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);
>
> Should we add:
>
> if (!type)
> type = GPMI_MXS;
>
> here?
(enum gpmi_type)0 is already GPMI_MXS. So that's a no-op.
>
>> 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
>>
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] mtd: nand-mxs: fix out-of-bounds write on 64-bit SoCs
2020-09-28 16:03 ` Ahmad Fatoum
@ 2020-09-28 16:08 ` Marco Felsch
0 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-09-28 16:08 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 20-09-28 18:03, Ahmad Fatoum wrote:
>
>
> On 9/28/20 6:01 PM, Marco Felsch wrote:
> > On 20-09-28 17:42, Ahmad Fatoum wrote:
> >> 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.
> >>
> >> 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);
> >
> > Should we add:
> >
> > if (!type)
> > type = GPMI_MXS;
> >
> > here?
>
> (enum gpmi_type)0 is already GPMI_MXS. So that's a no-op.
I see, just wanted to be sure :)
Regards,
Marco
>
> >
> >> 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
> >>
> >
>
> --
> 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
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
` (6 preceding siblings ...)
2020-09-28 15:46 ` [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
@ 2020-09-29 6:45 ` Sascha Hauer
2020-09-29 7:32 ` Sascha Hauer
8 siblings, 0 replies; 20+ messages in thread
From: Sascha Hauer @ 2020-09-29 6:45 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Mon, Sep 28, 2020 at 05:42:41PM +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);
>
> 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
but what?
Sascha
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/7] led: pca955x: fix probing from device tree
2020-09-28 15:42 ` [PATCH 2/7] led: pca955x: fix probing from device tree Ahmad Fatoum
@ 2020-09-29 7:19 ` Sascha Hauer
2020-09-30 7:28 ` Ahmad Fatoum
0 siblings, 1 reply; 20+ messages in thread
From: Sascha Hauer @ 2020-09-29 7:19 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Mon, Sep 28, 2020 at 05:42:42PM +0200, Ahmad Fatoum wrote:
> 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.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/led/led-pca955x.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/led/led-pca955x.c b/drivers/led/led-pca955x.c
> index 27fefce8d524..f89fcbfba5ac 100644
> --- a/drivers/led/led-pca955x.c
> +++ b/drivers/led/led-pca955x.c
> @@ -349,8 +349,11 @@ static int led_pca955x_probe(struct device_d *dev)
> struct i2c_client *client;
> int err;
> struct pca955x_platform_data *pdata;
> + enum pca955x_type type;
>
> - chip = &pca955x_chipdefs[dev->id_entry->driver_data];
> + type = (enum pca955x_type)device_get_match_data(dev);
In this driver we have this:
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,
},
};
So instead of putting the enum casted to void pointer into the matchdata
we could put pointers to struct pca955x_chipdef directly into the
matchdata.
Sascha
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
` (7 preceding siblings ...)
2020-09-29 6:45 ` Sascha Hauer
@ 2020-09-29 7:32 ` Sascha Hauer
2020-09-29 9:42 ` Ahmad Fatoum
8 siblings, 1 reply; 20+ messages in thread
From: Sascha Hauer @ 2020-09-29 7:32 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Mon, Sep 28, 2020 at 05:42:41PM +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);
>
> 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
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> 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;
> +}
Can we please give the caller a possibility to distinguish between
errors and a valid NULL pointer?
As you realize in your series some drivers cast the returned value into
an integer type and use 0 as a valid possibility. These need an extra
explanation why we can accept that case. I can think of different
possibilies to get that straight:
- Put a real pointer into matchdata. This is really preferred as it
motivates people to put flags into a (struct type) matchdata which
doesn't lead to excessive if (type == foo || type == bar || type ==
baz) we sometimes see in drivers.
- Return an ERR_PTR from device_get_match_data(). this is less likely
interpreted as a valid int value
- add a int * ret argument to device_get_match_data() which returns
the error code.
Sascha
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-09-29 7:32 ` Sascha Hauer
@ 2020-09-29 9:42 ` Ahmad Fatoum
2020-09-30 7:48 ` Sascha Hauer
0 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-29 9:42 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Hello Sascha,
On 9/29/20 9:32 AM, Sascha Hauer wrote:
> On Mon, Sep 28, 2020 at 05:42:41PM +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);
>>
>> 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
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> 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;
>> +}
>
> Can we please give the caller a possibility to distinguish between
> errors and a valid NULL pointer?
The only error possible is that we matched the device by name
and not of_compatible or platform id entry.
If the match data is a valid pointer:
-> It doesn't matter, why we have no match data either way.
If the match data is a casted integer (e.g. enum):
The driver author should either:
-> place the default enum value as first one,
so no match data => default
-> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum
and handle it appropriately
I like the function signature like that, I don't really see
a need to adjust it.
> As you realize in your series some drivers cast the returned value into
> an integer type and use 0 as a valid possibility. These need an extra
> explanation why we can accept that case. I can think of different
> possibilies to get that straight:
>
> - Put a real pointer into matchdata. This is really preferred as it
> motivates people to put flags into a (struct type) matchdata which
> doesn't lead to excessive if (type == foo || type == bar || type ==
> baz) we sometimes see in drivers.
We have a real pointer there already. The problem is migrating the
existing drivers.
> - Return an ERR_PTR from device_get_match_data(). this is less likely
> interpreted as a valid int value
Doesn't cover all cases. Also for the normal use, it means
you need to have to check with IS_ERR_OR_NULL everywhere to
be sure you don't dereference a NULL pointer.
> - add a int * ret argument to device_get_match_data() which returns
> the error code.
Would work, but my preference is leaving it to the drivers.
The drivers need to handle errors anyway.
>
> Sascha
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/7] led: pca955x: fix probing from device tree
2020-09-29 7:19 ` Sascha Hauer
@ 2020-09-30 7:28 ` Ahmad Fatoum
0 siblings, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-30 7:28 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Hi,
On 9/29/20 9:19 AM, Sascha Hauer wrote:
> On Mon, Sep 28, 2020 at 05:42:42PM +0200, Ahmad Fatoum wrote:
>> 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.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> drivers/led/led-pca955x.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/led/led-pca955x.c b/drivers/led/led-pca955x.c
>> index 27fefce8d524..f89fcbfba5ac 100644
>> --- a/drivers/led/led-pca955x.c
>> +++ b/drivers/led/led-pca955x.c
>> @@ -349,8 +349,11 @@ static int led_pca955x_probe(struct device_d *dev)
>> struct i2c_client *client;
>> int err;
>> struct pca955x_platform_data *pdata;
>> + enum pca955x_type type;
>>
>> - chip = &pca955x_chipdefs[dev->id_entry->driver_data];
>> + type = (enum pca955x_type)device_get_match_data(dev);
>
> In this driver we have this:
>
> 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,
> },
> };
>
> So instead of putting the enum casted to void pointer into the matchdata
> we could put pointers to struct pca955x_chipdef directly into the
> matchdata.
platform id match data will have a cast as well, but casting pointer
to long is probably better than pointer to enum. Will send out along
with v2 when we reach agreement on the new function's signature.
>
> Sascha
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-09-29 9:42 ` Ahmad Fatoum
@ 2020-09-30 7:48 ` Sascha Hauer
2020-09-30 13:13 ` Ahmad Fatoum
0 siblings, 1 reply; 20+ messages in thread
From: Sascha Hauer @ 2020-09-30 7:48 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Sep 29, 2020 at 11:42:04AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
>
> On 9/29/20 9:32 AM, Sascha Hauer wrote:
> > On Mon, Sep 28, 2020 at 05:42:41PM +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);
> >>
> >> 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
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> 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;
> >> +}
> >
> > Can we please give the caller a possibility to distinguish between
> > errors and a valid NULL pointer?
>
> The only error possible is that we matched the device by name
> and not of_compatible or platform id entry.
>
> If the match data is a valid pointer:
> -> It doesn't matter, why we have no match data either way.
>
> If the match data is a casted integer (e.g. enum):
> The driver author should either:
> -> place the default enum value as first one,
> so no match data => default
You know that, but "The driver Author" probably doesn't.
> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum
> and handle it appropriately
>
> I like the function signature like that, I don't really see
> a need to adjust it.
>
> > As you realize in your series some drivers cast the returned value into
> > an integer type and use 0 as a valid possibility. These need an extra
> > explanation why we can accept that case. I can think of different
> > possibilies to get that straight:
> >
> > - Put a real pointer into matchdata. This is really preferred as it
> > motivates people to put flags into a (struct type) matchdata which
> > doesn't lead to excessive if (type == foo || type == bar || type ==
> > baz) we sometimes see in drivers.
>
> We have a real pointer there already. The problem is migrating the
> existing drivers.
Yes, existing drivers would have to be migrated, that is exactly what I
am proposing.
>
> > - Return an ERR_PTR from device_get_match_data(). this is less likely
> > interpreted as a valid int value
>
> Doesn't cover all cases. Also for the normal use, it means
> you need to have to check with IS_ERR_OR_NULL everywhere to
> be sure you don't dereference a NULL pointer.
Why that? Just don't return NULL when there's no match data, but return
-ESOMETHING.
Sascha
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-09-30 7:48 ` Sascha Hauer
@ 2020-09-30 13:13 ` Ahmad Fatoum
2020-10-05 8:31 ` Ahmad Fatoum
0 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-09-30 13:13 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Hi,
On 9/30/20 9:48 AM, Sascha Hauer wrote:
>> If the match data is a valid pointer:
>> -> It doesn't matter, why we have no match data either way.
>>
>> If the match data is a casted integer (e.g. enum):
>> The driver author should either:
>> -> place the default enum value as first one,
>> so no match data => default
>
> You know that, but "The driver Author" probably doesn't.
Like with all other functions that return an error code,
the author should check those errors:
enum lm75_type type = (enum lm75_type)device_get_match_data(dev);
if (type == 0)
return -ENODEV;
I'd expect they will see that 0 shouldn't be part of the enumeration.
>> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum
>> and handle it appropriately
>>
>> I like the function signature like that, I don't really see
>> a need to adjust it.
>>
>>> As you realize in your series some drivers cast the returned value into
>>> an integer type and use 0 as a valid possibility. These need an extra
>>> explanation why we can accept that case. I can think of different
>>> possibilies to get that straight:
>>>
>>> - Put a real pointer into matchdata. This is really preferred as it
>>> motivates people to put flags into a (struct type) matchdata which
>>> doesn't lead to excessive if (type == foo || type == bar || type ==
>>> baz) we sometimes see in drivers.
>>
>> We have a real pointer there already. The problem is migrating the
>> existing drivers.
>
> Yes, existing drivers would have to be migrated, that is exactly what I
> am proposing.
>
>>
>>> - Return an ERR_PTR from device_get_match_data(). this is less likely
>>> interpreted as a valid int value
>>
>> Doesn't cover all cases. Also for the normal use, it means
>> you need to have to check with IS_ERR_OR_NULL everywhere to
>> be sure you don't dereference a NULL pointer.
>
> Why that? Just don't return NULL when there's no match data, but return
> -ESOMETHING.
For the lm75 we would have to do:
enum lm75_type { TYPE_1, TYPE_2 };
const void *match = device_get_match_data(dev);
if (IS_ERR(match))
return PTR_ERR(match);
enum lm75_type type = (enum lm75_type)match;
The alternative being:
enum lm75_type { TYPE_UNKNOWN = 0, TYPE_1, TYPE_2 };
enum lm75_type type = (enum lm75_type)device_get_match_data(dev);
if (type == TYPE_UNKNOWN)
return -ENODEV;
I prefer the second one very much.
>
> Sascha
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-09-30 13:13 ` Ahmad Fatoum
@ 2020-10-05 8:31 ` Ahmad Fatoum
2020-10-07 8:43 ` Sascha Hauer
0 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-10-05 8:31 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Hello Sascha,
On 9/30/20 3:13 PM, Ahmad Fatoum wrote:
> Hi,
>
> On 9/30/20 9:48 AM, Sascha Hauer wrote:
>>> If the match data is a valid pointer:
>>> -> It doesn't matter, why we have no match data either way.
>>>
>>> If the match data is a casted integer (e.g. enum):
>>> The driver author should either:
>>> -> place the default enum value as first one,
>>> so no match data => default
>>
>> You know that, but "The driver Author" probably doesn't.
>
> Like with all other functions that return an error code,
> the author should check those errors:
>
> enum lm75_type type = (enum lm75_type)device_get_match_data(dev);
> if (type == 0)
> return -ENODEV;
>
> I'd expect they will see that 0 shouldn't be part of the enumeration.
>
>>> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum
>>> and handle it appropriately
>>>
>>> I like the function signature like that, I don't really see
>>> a need to adjust it.
>>>
>>>> As you realize in your series some drivers cast the returned value into
>>>> an integer type and use 0 as a valid possibility. These need an extra
>>>> explanation why we can accept that case. I can think of different
>>>> possibilies to get that straight:
>>>>
>>>> - Put a real pointer into matchdata. This is really preferred as it
>>>> motivates people to put flags into a (struct type) matchdata which
>>>> doesn't lead to excessive if (type == foo || type == bar || type ==
>>>> baz) we sometimes see in drivers.
>>>
>>> We have a real pointer there already. The problem is migrating the
>>> existing drivers.
>>
>> Yes, existing drivers would have to be migrated, that is exactly what I
>> am proposing.
>>
>>>
>>>> - Return an ERR_PTR from device_get_match_data(). this is less likely
>>>> interpreted as a valid int value
>>>
>>> Doesn't cover all cases. Also for the normal use, it means
>>> you need to have to check with IS_ERR_OR_NULL everywhere to
>>> be sure you don't dereference a NULL pointer.
>>
>> Why that? Just don't return NULL when there's no match data, but return
>> -ESOMETHING.
>
> For the lm75 we would have to do:
>
> enum lm75_type { TYPE_1, TYPE_2 };
>
> const void *match = device_get_match_data(dev);
> if (IS_ERR(match))
> return PTR_ERR(match);
> enum lm75_type type = (enum lm75_type)match;
>
> The alternative being:
>
> enum lm75_type { TYPE_UNKNOWN = 0, TYPE_1, TYPE_2 };
>
> enum lm75_type type = (enum lm75_type)device_get_match_data(dev);
> if (type == TYPE_UNKNOWN)
> return -ENODEV;
>
> I prefer the second one very much.
Gentle ping.
>
>>
>> Sascha
>>
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative
2020-10-05 8:31 ` Ahmad Fatoum
@ 2020-10-07 8:43 ` Sascha Hauer
0 siblings, 0 replies; 20+ messages in thread
From: Sascha Hauer @ 2020-10-07 8:43 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Mon, Oct 05, 2020 at 10:31:32AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
>
> On 9/30/20 3:13 PM, Ahmad Fatoum wrote:
> > Hi,
> >
> > On 9/30/20 9:48 AM, Sascha Hauer wrote:
> >>> If the match data is a valid pointer:
> >>> -> It doesn't matter, why we have no match data either way.
> >>>
> >>> If the match data is a casted integer (e.g. enum):
> >>> The driver author should either:
> >>> -> place the default enum value as first one,
> >>> so no match data => default
> >>
> >> You know that, but "The driver Author" probably doesn't.
> >
> > Like with all other functions that return an error code,
> > the author should check those errors:
> >
> > enum lm75_type type = (enum lm75_type)device_get_match_data(dev);
> > if (type == 0)
> > return -ENODEV;
> >
> > I'd expect they will see that 0 shouldn't be part of the enumeration.
> >
> >>> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum
> >>> and handle it appropriately
> >>>
> >>> I like the function signature like that, I don't really see
> >>> a need to adjust it.
> >>>
> >>>> As you realize in your series some drivers cast the returned value into
> >>>> an integer type and use 0 as a valid possibility. These need an extra
> >>>> explanation why we can accept that case. I can think of different
> >>>> possibilies to get that straight:
> >>>>
> >>>> - Put a real pointer into matchdata. This is really preferred as it
> >>>> motivates people to put flags into a (struct type) matchdata which
> >>>> doesn't lead to excessive if (type == foo || type == bar || type ==
> >>>> baz) we sometimes see in drivers.
> >>>
> >>> We have a real pointer there already. The problem is migrating the
> >>> existing drivers.
> >>
> >> Yes, existing drivers would have to be migrated, that is exactly what I
> >> am proposing.
> >>
> >>>
> >>>> - Return an ERR_PTR from device_get_match_data(). this is less likely
> >>>> interpreted as a valid int value
> >>>
> >>> Doesn't cover all cases. Also for the normal use, it means
> >>> you need to have to check with IS_ERR_OR_NULL everywhere to
> >>> be sure you don't dereference a NULL pointer.
> >>
> >> Why that? Just don't return NULL when there's no match data, but return
> >> -ESOMETHING.
> >
> > For the lm75 we would have to do:
> >
> > enum lm75_type { TYPE_1, TYPE_2 };
> >
> > const void *match = device_get_match_data(dev);
> > if (IS_ERR(match))
> > return PTR_ERR(match);
> > enum lm75_type type = (enum lm75_type)match;
> >
> > The alternative being:
> >
> > enum lm75_type { TYPE_UNKNOWN = 0, TYPE_1, TYPE_2 };
> >
> > enum lm75_type type = (enum lm75_type)device_get_match_data(dev);
> > if (type == TYPE_UNKNOWN)
> > return -ENODEV;
> >
> > I prefer the second one very much.
ok then, let's go for it.
Sascha
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-10-07 8:43 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 15:42 [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 2/7] led: pca955x: fix probing from device tree Ahmad Fatoum
2020-09-29 7:19 ` Sascha Hauer
2020-09-30 7:28 ` Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 3/7] dma: apbh: fix out-of-bounds write on 64-bit SoCs Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 4/7] aiodev: lm75: " Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 5/7] mtd: nand-mxs: " Ahmad Fatoum
2020-09-28 16:01 ` Marco Felsch
2020-09-28 16:03 ` Ahmad Fatoum
2020-09-28 16:08 ` Marco Felsch
2020-09-28 15:42 ` [PATCH 6/7] video: imx-hdmi: fix dev_get_drvdata misuse Ahmad Fatoum
2020-09-28 15:42 ` [PATCH 7/7] driver: migrate some from dev_get_drvdata to device_get_match_data Ahmad Fatoum
2020-09-28 15:46 ` [PATCH 1/7] driver: introduce less error-prone dev_get_drvdata alternative Ahmad Fatoum
2020-09-29 6:45 ` Sascha Hauer
2020-09-29 7:32 ` Sascha Hauer
2020-09-29 9:42 ` Ahmad Fatoum
2020-09-30 7:48 ` Sascha Hauer
2020-09-30 13:13 ` Ahmad Fatoum
2020-10-05 8:31 ` Ahmad Fatoum
2020-10-07 8:43 ` Sascha Hauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox