mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [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