Skip to contentSkip to conversation

Add dev_warn_probe() and improve error handling in Rockchip SPI drivers

PATCH In review Dragan Simic 4 participants 17 messages Sep 29, 2024 – Oct 10, 2024

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v3 cover Add dev_warn_probe() and improve error handling in Rockchip SPI drivers

This is a small series that introduces dev_warn_probe() function, which produces warnings on failed resource acquisitions, and improves error handling in the probe paths of Rockchip SPI drivers, by using functions dev_err_probe() and dev_warn_probe() properly in multiple places.

This series also performs a bunch of small, rather trivial code cleanups, to make the code neater and a bit easier to read.

Changes in v3:
- Fixed a couple of rather embarrassing issues in patch 4/5 pointed out by the kernel test robot, [7] one of them using the va_copy() workaround approach already established in the commit 3fbcf75bb41a ("efi/printf: Factor out width/precision parsing"), but all this really made me think how didn't I notice those issues myself before sending the patches, which perhaps slipped by because I didn't take the specifics of x86_64 into account :/
- Added a somewhat lengthy comment to patch 4/5, to explain the newly introduced va_copy() workaround, partially reusing the comment found in the above-mentioned commit 3fbcf75bb41a
- Added underscores to the name of the "worker" function in patch 4/5, to additionally emphasize its internal nature
- Adjusted some of the variable names in patch 4/5 a bit to use what are seemingly more commonly used names

Changes in v2:
- Collected three Reviewed-by tags from Heiko [1][2][3]
- Dropped patch 3/5, [4] as suggested by Mark, [5] improved the check to use dev_err_probe() and folded that into new patch 5/5
- Added new patch 4/5 that introduces function dev_warn_probe() that produces warnings in probe paths, to avoid the promotion of logged messages from warnings to errors, as noted by Heiko [6]
- Adjusted the description of the series and of the individual patches a bit to reflect the changes, where appropriate

Link to v2: https://lore.kernel.org/linux-rockchip/[email protected]/T/#u
Link to v1: https://lore.kernel.org/linux-rockchip/[email protected]/T/#u

[1] https://lore.kernel.org/linux-rockchip/6085918.31tnzDBltd@phil/
[2] https://lore.kernel.org/linux-rockchip/2285557.3ZeAukHxDK@phil/
[3] https://lore.kernel.org/linux-rockchip/10409403.0AQdONaE2F@phil/
[4] https://lore.kernel.org/linux-rockchip/ce2e7f90e62b15adc2bed1f53122ad39c3a9b5ac.1727337732.git.dsimic@manjaro.org/
[5] https://lore.kernel.org/linux-rockchip/[email protected]/
[6] https://lore.kernel.org/linux-rockchip/6673004.tM3a2QDmDi@phil/
[7] https://lore.kernel.org/linux-rockchip/[email protected]/

Dragan Simic (5):
spi: rockchip: Perform trivial code cleanups
spi: rockchip-sfc: Perform trivial code cleanups
spi: rockchip-sfc: Use dev_err_probe() in the probe path driver core: Add device probe log helper dev_warn_probe()
spi: rockchip: Use dev_{err,warn}_probe() in the probe path

 drivers/base/core.c            | 129 ++++++++++++++++++++++++++-------
 drivers/spi/spi-rockchip-sfc.c |  21 ++----
 drivers/spi/spi-rockchip.c     |  55 +++++++-------
 include/linux/dev_printk.h     |   1 +
 4 files changed, 135 insertions(+), 71 deletions(-)
#1 lore raw

PATCH v3 1/5 spi: rockchip: Perform trivial code cleanups

Perform a few trivial code cleanups, to obey the reverse Christmas tree rule, to avoid unnecessary line wrapping by using the 100-column width better, to actually obey the 100-column width in one case, and to make the way a couple of wrapped function arguments are indented a bit more readable.

No intended functional changes are introduced by these code cleanups.

---
 drivers/spi/spi-rockchip.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)
drivers/spi/spi-rockchip.c +12 -15
index e1ecd96c7858..81f2a966c124 100644
@@ -742,34 +742,32 @@ static int rockchip_spi_setup(struct spi_device *spi)
 
 static int rockchip_spi_probe(struct platform_device *pdev)
 {
-	int ret;
-	struct rockchip_spi *rs;
+	struct device_node *np = pdev->dev.of_node;
 	struct spi_controller *ctlr;
+	struct rockchip_spi *rs;
 	struct resource *mem;
-	struct device_node *np = pdev->dev.of_node;
 	u32 rsd_nsecs, num_cs;
 	bool target_mode;
+	int ret;
 
 	target_mode = of_property_read_bool(np, "spi-slave");
 
 	if (target_mode)
-		ctlr = spi_alloc_target(&pdev->dev,
-				sizeof(struct rockchip_spi));
+		ctlr = spi_alloc_target(&pdev->dev, sizeof(struct rockchip_spi));
 	else
-		ctlr = spi_alloc_host(&pdev->dev,
-				sizeof(struct rockchip_spi));
+		ctlr = spi_alloc_host(&pdev->dev, sizeof(struct rockchip_spi));
 
 	if (!ctlr)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, ctlr);
 
 	rs = spi_controller_get_devdata(ctlr);
 
 	/* Get basic io resource and map it */
 	rs->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
 	if (IS_ERR(rs->regs)) {
-		ret =  PTR_ERR(rs->regs);
+		ret = PTR_ERR(rs->regs);
 		goto err_put_ctlr;
 	}
 
@@ -794,26 +792,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		goto err_put_ctlr;
 
 	ret = devm_request_threaded_irq(&pdev->dev, ret, rockchip_spi_isr, NULL,
-			IRQF_ONESHOT, dev_name(&pdev->dev), ctlr);
+					IRQF_ONESHOT, dev_name(&pdev->dev), ctlr);
 	if (ret)
 		goto err_put_ctlr;
 
 	rs->dev = &pdev->dev;
 	rs->freq = clk_get_rate(rs->spiclk);
 
 	if (!of_property_read_u32(pdev->dev.of_node, "rx-sample-delay-ns",
 				  &rsd_nsecs)) {
 		/* rx sample delay is expressed in parent clock cycles (max 3) */
-		u32 rsd = DIV_ROUND_CLOSEST(rsd_nsecs * (rs->freq >> 8),
-				1000000000 >> 8);
+		u32 rsd = DIV_ROUND_CLOSEST(rsd_nsecs * (rs->freq >> 8), 1000000000 >> 8);
 		if (!rsd) {
 			dev_warn(rs->dev, "%u Hz are too slow to express %u ns delay\n",
-					rs->freq, rsd_nsecs);
+				 rs->freq, rsd_nsecs);
 		} else if (rsd > CR0_RSD_MAX) {
 			rsd = CR0_RSD_MAX;
-			dev_warn(rs->dev, "%u Hz are too fast to express %u ns delay, clamping at %u ns\n",
-					rs->freq, rsd_nsecs,
-					CR0_RSD_MAX * 1000000000U / rs->freq);
+			dev_warn(rs->dev,
+				 "%u Hz are too fast to express %u ns delay, clamping at %u ns\n",
+				 rs->freq, rsd_nsecs, CR0_RSD_MAX * 1000000000U / rs->freq);
 		}
 		rs->rsd = rsd;
 	}
#2 lore raw

PATCH v3 2/5 spi: rockchip-sfc: Perform trivial code cleanups

In reply to Dragan Simic

Perform a couple of trivial code cleanups, to avoid unnecessary line wrapping by using the 100-column width a bit better, and to drop a stray empty line.

No intended functional changes are introduced by these code cleanups.

---
 drivers/spi/spi-rockchip-sfc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/spi/spi-rockchip-sfc.c +2 -5
index 0d7fadcd4ed3..505d5089bf03 100644
@@ -591,19 +591,17 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 		return PTR_ERR(sfc->hclk);
 	}
 
-	sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
-					      "rockchip,sfc-no-dma");
+	sfc->use_dma = !of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma");
 
 	if (sfc->use_dma) {
 		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 		if (ret) {
 			dev_warn(dev, "Unable to set dma mask\n");
 			return ret;
 		}
 
 		sfc->buffer = dmam_alloc_coherent(dev, SFC_MAX_IOSIZE_VER3,
-						  &sfc->dma_buffer,
-						  GFP_KERNEL);
+						  &sfc->dma_buffer, GFP_KERNEL);
 		if (!sfc->buffer)
 			return -ENOMEM;
 	}
@@ -629,7 +627,6 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 			       0, pdev->name, sfc);
 	if (ret) {
 		dev_err(dev, "Failed to request irq\n");
-
 		goto err_irq;
 	}
 
#3 lore raw

PATCH v3 3/5 spi: rockchip-sfc: Use dev_err_probe() in the probe path

In reply to Dragan Simic

Use function dev_err_probe() in the probe path instead of dev_err() where appropriate, to make the code a bit more uniform and compact.

---
 drivers/spi/spi-rockchip-sfc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/spi/spi-rockchip-sfc.c +6 -8
index 505d5089bf03..7e0fb4944a34 100644
@@ -580,16 +580,14 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 		return PTR_ERR(sfc->regbase);
 
 	sfc->clk = devm_clk_get(&pdev->dev, "clk_sfc");
-	if (IS_ERR(sfc->clk)) {
-		dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
-		return PTR_ERR(sfc->clk);
-	}
+	if (IS_ERR(sfc->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sfc->clk),
+				     "Failed to get sfc interface clk\n");
 
 	sfc->hclk = devm_clk_get(&pdev->dev, "hclk_sfc");
-	if (IS_ERR(sfc->hclk)) {
-		dev_err(&pdev->dev, "Failed to get sfc ahb clk\n");
-		return PTR_ERR(sfc->hclk);
-	}
+	if (IS_ERR(sfc->hclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sfc->hclk),
+				     "Failed to get sfc ahb clk\n");
 
 	sfc->use_dma = !of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma");
 
#4 lore raw

PATCH v3 4/5 driver core: Add device probe log helper dev_warn_probe()

In reply to Dragan Simic

Some drivers can still provide their functionality to a certain extent even some of their resource acquisitions eventually fail. In such cases, emitting errors isn't the desired action, but warnings should be emitted instead.

To solve this, introduce dev_warn_probe() as a new device probe log helper, which behaves identically as the already existing dev_err_probe(), while it produces warnings instead of errors. The intended use is with the resources that are actually optional for a particular driver.

While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with the necessary wording adjustments, of course.

---
 drivers/base/core.c        | 129 +++++++++++++++++++++++++++++--------
 include/linux/dev_printk.h |   1 +
 2 files changed, 102 insertions(+), 28 deletions(-)
drivers/base/core.c +101 -28
index 8c0733d3aad8..f2e41db0c09f 100644
@@ -4982,71 +4982,144 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
+			       const char *fmt, va_list vargsp)
+{
+	struct va_format vaf;
+	va_list vargs;
+
+	/*
+	 * On x86_64 and possibly on other architectures, va_list is actually a
+	 * size-1 array containing a structure.  As a result, function parameter
+	 * vargps decays from T[1] to T*, and &vargsp has type T** rather than
+	 * T(*)[1], which is expected by its assignment to vaf.va below.
+	 *
+	 * One standard way to solve this mess is by creating a copy in a local
+	 * variable of type va_list and then using a pointer to that local copy
+	 * instead, which is the approach employed here.
+	 */
+	va_copy(vargs, vargsp);
+
+	vaf.fmt = fmt;
+	vaf.va = &vargs;
+
+	switch (err) {
+	case -EPROBE_DEFER:
+		device_set_deferred_probe_reason(dev, &vaf);
+		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+		break;
+
+	case -ENOMEM:
+		/* Don't print anything on -ENOMEM, there's already enough output */
+		break;
+
+	default:
+		/* Log fatal final failures as errors, otherwise produce warnings */
+		if (fatal)
+			dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+		else
+			dev_warn(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+		break;
+	}
+
+	va_end(vargs);
+}
+
 /**
  * dev_err_probe - probe error check and log helper
  * @dev: the pointer to the struct device
  * @err: error value to test
  * @fmt: printf-style format string
  * @...: arguments as specified in the format string
  *
  * This helper implements common pattern present in probe functions for error
  * checking: print debug or error message depending if the error value is
  * -EPROBE_DEFER and propagate error upwards.
  * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
  * checked later by reading devices_deferred debugfs attribute.
- * It replaces code sequence::
+ * It replaces the following code sequence::
  *
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
  * 	else
  * 		dev_dbg(dev, ...);
  * 	return err;
  *
  * with::
  *
  * 	return dev_err_probe(dev, err, ...);
  *
- * Using this helper in your probe function is totally fine even if @err is
- * known to never be -EPROBE_DEFER.
+ * Using this helper in your probe function is totally fine even if @err
+ * is known to never be -EPROBE_DEFER.
  * The benefit compared to a normal dev_err() is the standardized format
- * of the error code, it being emitted symbolically (i.e. you get "EAGAIN"
- * instead of "-35") and the fact that the error code is returned which allows
- * more compact error paths.
+ * of the error code, which is emitted symbolically (i.e. you get "EAGAIN"
+ * instead of "-35"), and having the error code returned allows more
+ * compact error paths.
  *
  * Returns @err.
  */
 int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 {
-	struct va_format vaf;
-	va_list args;
+	va_list vargs;
 
-	va_start(args, fmt);
-	vaf.fmt = fmt;
-	vaf.va = &args;
+	va_start(vargs, fmt);
 
-	switch (err) {
-	case -EPROBE_DEFER:
-		device_set_deferred_probe_reason(dev, &vaf);
-		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
-		break;
+	/* Use dev_err() for logging when err doesn't equal -EPROBE_DEFER */
+	__dev_probe_failed(dev, err, true, fmt, vargs);
 
-	case -ENOMEM:
-		/*
-		 * We don't print anything on -ENOMEM, there is already enough
-		 * output.
-		 */
-		break;
+	va_end(vargs);
 
-	default:
-		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
-		break;
-	}
+	return err;
+}
+EXPORT_SYMBOL_GPL(dev_err_probe);
 
-	va_end(args);
+/**
+ * dev_warn_probe - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print debug or warning message depending if the error value is
+ * -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
+ * It replaces the following code sequence::
+ *
+ * 	if (err != -EPROBE_DEFER)
+ * 		dev_warn(dev, ...);
+ * 	else
+ * 		dev_dbg(dev, ...);
+ * 	return err;
+ *
+ * with::
+ *
+ * 	return dev_warn_probe(dev, err, ...);
+ *
+ * Using this helper in your probe function is totally fine even if @err
+ * is known to never be -EPROBE_DEFER.
+ * The benefit compared to a normal dev_warn() is the standardized format
+ * of the error code, which is emitted symbolically (i.e. you get "EAGAIN"
+ * instead of "-35"), and having the error code returned allows more
+ * compact error paths.
+ *
+ * Returns @err.
+ */
+int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...)
+{
+	va_list vargs;
+
+	va_start(vargs, fmt);
+
+	/* Use dev_warn() for logging when err doesn't equal -EPROBE_DEFER */
+	__dev_probe_failed(dev, err, false, fmt, vargs);
+
+	va_end(vargs);
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(dev_err_probe);
+EXPORT_SYMBOL_GPL(dev_warn_probe);
 
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
include/linux/dev_printk.h +1 -0
index ca32b5bb28eb..eb2094e43050 100644
@@ -276,6 +276,7 @@ do {									\
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
 __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+__printf(3, 4) int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...);
 
 /* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
 #define dev_err_ptr_probe(dev, ___err, fmt, ...) \
Tested-by on v3 4/5
#5 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

Quoted 9 lines · Dragan Simic

Some drivers can still provide their functionality to a certain extent even some of their resource acquisitions eventually fail. In such cases, emitting errors isn't the desired action, but warnings should be emitted instead.

To solve this, introduce dev_warn_probe() as a new device probe log helper, which behaves identically as the already existing dev_err_probe(), while it produces warnings instead of errors. The intended use is with the resources that are actually optional for a particular driver.

Quoted 5 lines

While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with the necessary wording adjustments, of course.

Applied and tested on 6.11 for arm64 without noticing any issues.

maintainer
#6 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

In reply to Dragan Simic
Quoted 4 lines · Dragan Simic

Some drivers can still provide their functionality to a certain extent even some of their resource acquisitions eventually fail. In such cases, emitting errors isn't the desired action, but warnings should be emitted instead.

Quoted 8 lines

To solve this, introduce dev_warn_probe() as a new device probe log helper, which behaves identically as the already existing dev_err_probe(), while it produces warnings instead of errors. The intended use is with the resources that are actually optional for a particular driver.

While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with the necessary wording adjustments, of course.

Greg, this makes sense to me - are you OK with me applying it?

maintainer
#7 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

Quoted 10 lines · Mark Brown
Quoted 9 lines · Dragan Simic

Some drivers can still provide their functionality to a certain extent even some of their resource acquisitions eventually fail. In such cases, emitting errors isn't the desired action, but warnings should be emitted instead.

To solve this, introduce dev_warn_probe() as a new device probe log helper, which behaves identically as the already existing dev_err_probe(), while it produces warnings instead of errors. The intended use is with the resources that are actually optional for a particular driver.

Quoted 5 lines
Quoted 3 lines

While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with the necessary wording adjustments, of course.

Greg, this makes sense to me - are you OK with me applying it?

No objection from me, now sent a reviewed-by

maintainer Acked-by on v3 4/5
#8 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

In reply to Dragan Simic
Quoted 13 lines · Dragan Simic

Some drivers can still provide their functionality to a certain extent even some of their resource acquisitions eventually fail. In such cases, emitting errors isn't the desired action, but warnings should be emitted instead.

To solve this, introduce dev_warn_probe() as a new device probe log helper, which behaves identically as the already existing dev_err_probe(), while it produces warnings instead of errors. The intended use is with the resources that are actually optional for a particular driver.

While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with the necessary wording adjustments, of course.

Quoted 5 lines
---
 drivers/base/core.c        | 129 +++++++++++++++++++++++++++++--------
 include/linux/dev_printk.h |   1 +
 2 files changed, 102 insertions(+), 28 deletions(-)
#9 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

In reply to Dragan Simic

Hello Mark,

I just spotted a couple of small typos, noted below, and I hope you won't mind to apply the fixes by hand before applying this patch, please?

Quoted 1 line · Dragan Simic

Some drivers can still provide their functionality to a certain extent even

s/extent even/extent when/

Quoted 39 lines

some of their resource acquisitions eventually fail. In such cases, emitting errors isn't the desired action, but warnings should be emitted instead.

To solve this, introduce dev_warn_probe() as a new device probe log helper, which behaves identically as the already existing dev_err_probe(), while it produces warnings instead of errors. The intended use is with the resources that are actually optional for a particular driver.

While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with the necessary wording adjustments, of course.

---
 drivers/base/core.c        | 129 +++++++++++++++++++++++++++++--------
 include/linux/dev_printk.h |   1 +
 2 files changed, 102 insertions(+), 28 deletions(-)
drivers/base/core.c +8 -0
index 8c0733d3aad8..f2e41db0c09f 100644
@@ -4982,71 +4982,144 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+static void __dev_probe_failed(const struct device *dev, int err, bool 
 fatal,
+			       const char *fmt, va_list vargsp)
+{
+	struct va_format vaf;
+	va_list vargs;
+
+	/*
+	 * On x86_64 and possibly on other architectures, va_list is actually 

a

drivers/base/core.c +1 -0
+	 * size-1 array containing a structure.  As a result, function 

parameter

drivers/base/core.c +1 -0
+	 * vargps decays from T[1] to T*, and &vargsp has type T** rather 
Quoted 1 line

than

s/vargps decays/vargsp decays/

Quoted 12 lines
drivers/base/core.c +3 -0
+	 * T(*)[1], which is expected by its assignment to vaf.va below.
+	 *
+	 * One standard way to solve this mess is by creating a copy in a 

local

drivers/base/core.c +1 -0
+	 * variable of type va_list and then using a pointer to that local 

copy

drivers/base/core.c +6 -0
+	 * instead, which is the approach employed here.
+	 */
+	va_copy(vargs, vargsp);
+
+	vaf.fmt = fmt;
+	vaf.va = &vargs;
maintainer
#10 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

Quoted 2 lines · Dragan Simic

I just spotted a couple of small typos, noted below, and I hope you won't mind to apply the fixes by hand before applying this patch, please?

Sorry, your mail arrived after I'd already published the changes - please send an incremental patch for the one in the comments.

#11 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

Hello Mark,

Quoted 7 lines · Mark Brown
Quoted 2 lines · Dragan Simic

I just spotted a couple of small typos, noted below, and I hope you won't mind to apply the fixes by hand before applying this patch, please?

Sorry, your mail arrived after I'd already published the changes - please send an incremental patch for the one in the comments.

No worries. I just sent the incremental patch, [1] please have a look. Thanks for merging this patch series!

[1] https://lore.kernel.org/linux-spi/cec37f5568afaef8fca2d35bb01c90556ccbb4f4.1728408464.git.dsimic@manjaro.org/

maintainer
#12 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

Quoted 2 lines · Dragan Simic

No worries. I just sent the incremental patch, [1] please have a look. Thanks for merging this patch series!

Oh, sorry - actually now I look again I got this confused with another patch and it's still in my CI so I can actually fix things up before merging.

#13 lore raw

Re: [PATCH v3 4/5] driver core: Add device probe log helper dev_warn_probe()

Quoted 8 lines · Mark Brown
Quoted 2 lines · Dragan Simic

No worries. I just sent the incremental patch, [1] please have a look. Thanks for merging this patch series!

Oh, sorry - actually now I look again I got this confused with another patch and it's still in my CI so I can actually fix things up before merging.

Awesome, even better!

#14 lore raw

PATCH v3 5/5 spi: rockchip: Use dev_{err,warn}_probe() in the probe path

In reply to Dragan Simic

Use function dev_err_probe() in the probe path instead of dev_err() where appropriate, to make the code a bit more uniform and compact. Use the new function dev_warn_probe() to improve error handling for the TX and RX DMA channel requests, which are actually optional, and tweak the logged warnings a bit to additionally describe their optional nature.

Previously, deferred requests for the TX and RX DMA channels produced no debug messages, and the final error messages didn't include the error codes, which are all highly useful when debugging permanently failed DMA channel requests, such as when the required drivers aren't enabled.

---
 drivers/spi/spi-rockchip.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)
drivers/spi/spi-rockchip.c +13 -15
index 81f2a966c124..055cd6066466 100644
@@ -773,15 +773,15 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 
 	rs->apb_pclk = devm_clk_get_enabled(&pdev->dev, "apb_pclk");
 	if (IS_ERR(rs->apb_pclk)) {
-		dev_err(&pdev->dev, "Failed to get apb_pclk\n");
-		ret = PTR_ERR(rs->apb_pclk);
+		ret = dev_err_probe(&pdev->dev, PTR_ERR(rs->apb_pclk),
+				    "Failed to get apb_pclk\n");
 		goto err_put_ctlr;
 	}
 
 	rs->spiclk = devm_clk_get_enabled(&pdev->dev, "spiclk");
 	if (IS_ERR(rs->spiclk)) {
-		dev_err(&pdev->dev, "Failed to get spi_pclk\n");
-		ret = PTR_ERR(rs->spiclk);
+		ret = dev_err_probe(&pdev->dev, PTR_ERR(rs->spiclk),
+				    "Failed to get spi_pclk\n");
 		goto err_put_ctlr;
 	}
 
@@ -817,8 +817,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 
 	rs->fifo_len = get_fifo_len(rs);
 	if (!rs->fifo_len) {
-		dev_err(&pdev->dev, "Failed to get fifo length\n");
-		ret = -EINVAL;
+		ret = dev_err_probe(&pdev->dev, -EINVAL, "Failed to get fifo length\n");
 		goto err_put_ctlr;
 	}
 
@@ -858,22 +857,21 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 
 	ctlr->dma_tx = dma_request_chan(rs->dev, "tx");
 	if (IS_ERR(ctlr->dma_tx)) {
-		/* Check tx to see if we need defer probing driver */
-		if (PTR_ERR(ctlr->dma_tx) == -EPROBE_DEFER) {
-			ret = -EPROBE_DEFER;
+		/* Check tx to see if we need to defer driver probing */
+		ret = dev_warn_probe(rs->dev, PTR_ERR(ctlr->dma_tx),
+				     "Failed to request optional TX DMA channel\n");
+		if (ret == -EPROBE_DEFER)
 			goto err_disable_pm_runtime;
-		}
-		dev_warn(rs->dev, "Failed to request TX DMA channel\n");
 		ctlr->dma_tx = NULL;
 	}
 
 	ctlr->dma_rx = dma_request_chan(rs->dev, "rx");
 	if (IS_ERR(ctlr->dma_rx)) {
-		if (PTR_ERR(ctlr->dma_rx) == -EPROBE_DEFER) {
-			ret = -EPROBE_DEFER;
+		/* Check rx to see if we need to defer driver probing */
+		ret = dev_warn_probe(rs->dev, PTR_ERR(ctlr->dma_rx),
+				     "Failed to request optional RX DMA channel\n");
+		if (ret == -EPROBE_DEFER)
 			goto err_free_dma_tx;
-		}
-		dev_warn(rs->dev, "Failed to request RX DMA channel\n");
 		ctlr->dma_rx = NULL;
 	}
 
Tested-by on v3 5/5
#15 lore raw

Re: [PATCH v3 5/5] spi: rockchip: Use dev_{err,warn}_probe() in the probe path

Quoted 6 lines · Dragan Simic

Use function dev_err_probe() in the probe path instead of dev_err() where appropriate, to make the code a bit more uniform and compact. Use the new function dev_warn_probe() to improve error handling for the TX and RX DMA channel requests, which are actually optional, and tweak the logged warnings a bit to additionally describe their optional nature.

Quoted 7 lines

Previously, deferred requests for the TX and RX DMA channels produced no debug messages, and the final error messages didn't include the error codes, which are all highly useful when debugging permanently failed DMA channel requests, such as when the required drivers aren't enabled.

I can now see appropriate messages for pending probes on Rockchip SPI devices, instead of "(reason unknown)", which is incredibly helpful for debugging DeviceTrees and kernel configs.

Tested on 6.11 and looks like it works as intended.

Thank you for this patch!

maintainer
#16 lore raw
In reply to Dragan Simic
Quoted 9 lines · Dragan Simic

This is a small series that introduces dev_warn_probe() function, which produces warnings on failed resource acquisitions, and improves error handling in the probe paths of Rockchip SPI drivers, by using functions dev_err_probe() and dev_warn_probe() properly in multiple places.

This series also performs a bunch of small, rather trivial code cleanups, to make the code neater and a bit easier to read.

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/5] spi: rockchip: Perform trivial code cleanups

      commit: f7bc15211fc6946203dd7e57c123f1e387d7225b
[2/5] spi: rockchip-sfc: Perform trivial code cleanups
      commit: cb91287b3b6d42e66f948fbc304f771792c2852f
[3/5] spi: rockchip-sfc: Use dev_err_probe() in the probe path
      commit: 7d46b8d8d78338a2ad986eec0790ddb22fad23a8
[4/5] driver core: Add device probe log helper dev_warn_probe()
      commit: 36e69b160705b65bf136c2fb6a1194447eeb8478
[5/5] spi: rockchip: Use dev_{err,warn}_probe() in the probe path
      commit: e2fc05873905f2ee96b38a116ae86f45fe7d8e49

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark