[PATCH v3 0/5] Add dev_warn_probe() and improve error handling in Rockchip SPI drivers

[PATCH v3 0/5] Add dev_warn_probe() and improve error handling in Rockchip SPI drivers

[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.

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/cover.1727496560.git.dsimic@manjaro.org/T/#u
Link to v1: https://lore.kernel.org/linux-rockchip/cover.1727337732.git.dsimic@manjaro.org/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/ZvUmk48R4hZYlO71@finisterre.sirena.org.uk/
[6] https://lore.kernel.org/linux-rockchip/6673004.tM3a2QDmDi@phil/
[7] https://lore.kernel.org/linux-rockchip/202409290910.55WdSCMH-lkp@intel.com/

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(-)

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

[Dragan Simic]

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.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index e1ecd96c7858..81f2a966c124 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -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;
 	}

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

[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.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip-sfc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip-sfc.c b/drivers/spi/spi-rockchip-sfc.c
index 0d7fadcd4ed3..505d5089bf03 100644
--- a/drivers/spi/spi-rockchip-sfc.c
+++ b/drivers/spi/spi-rockchip-sfc.c
@@ -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;
 	}
 

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

[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.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip-sfc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-rockchip-sfc.c b/drivers/spi/spi-rockchip-sfc.c
index 505d5089bf03..7e0fb4944a34 100644
--- a/drivers/spi/spi-rockchip-sfc.c
+++ b/drivers/spi/spi-rockchip-sfc.c
@@ -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");
 

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

[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.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/base/core.c        | 129 +++++++++++++++++++++++++++++--------
 include/linux/dev_printk.h |   1 +
 2 files changed, 102 insertions(+), 28 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8c0733d3aad8..f2e41db0c09f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -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)
 {
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index ca32b5bb28eb..eb2094e43050 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -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, ...) \

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

[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.

Suggested-by: Hélene Vulquin <oss@helene.moe>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 81f2a966c124..055cd6066466 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -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;
 	}
 

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

[Hélène Vulquin]

On Sun Sep 29, 2024 at 11:21 AM CEST, Dragan Simic wrote:
> 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.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

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

Tested-by: Hélène Vulquin <oss@helene.moe>

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

[Hélène Vulquin]

On Sun Sep 29, 2024 at 11:21 AM CEST, Dragan Simic wrote:
> 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.
>
> Suggested-by: Hélene Vulquin <oss@helene.moe>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

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!

Tested-by: Hélène Vulquin <oss@helene.moe>

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Sun, Sep 29, 2024 at 11:21:16AM +0200, Dragan Simic wrote:
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Greg KH]

On Sun, Sep 29, 2024 at 11:21:16AM +0200, Dragan Simic wrote:
> 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.
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  drivers/base/core.c        | 129 +++++++++++++++++++++++++++++--------
>  include/linux/dev_printk.h |   1 +
>  2 files changed, 102 insertions(+), 28 deletions(-)


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Greg KH]

On Mon, Oct 07, 2024 at 03:25:52PM +0100, Mark Brown wrote:
> On Sun, Sep 29, 2024 at 11:21:16AM +0200, Dragan Simic wrote:
> > 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.
> 
> Greg, this makes sense to me - are you OK with me applying it?

No objection from me, now sent a reviewed-by

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

[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?

On 2024-09-29 11:21, Dragan Simic wrote:
> Some drivers can still provide their functionality to a certain extent 
> even

s/extent even/extent when/

> 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.
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  drivers/base/core.c        | 129 +++++++++++++++++++++++++++++--------
>  include/linux/dev_printk.h |   1 +
>  2 files changed, 102 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 8c0733d3aad8..f2e41db0c09f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -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

s/vargps decays/vargsp decays/

> +	 * 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;

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

On Tue, Oct 08, 2024 at 06:18:46PM +0200, Dragan Simic wrote:

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Dragan Simic]

Hello Mark,

On 2024-10-08 19:00, Mark Brown wrote:
> On Tue, Oct 08, 2024 at 06:18:46PM +0200, Dragan Simic wrote:
> 
>> 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/

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

On Tue, Oct 08, 2024 at 07:32:18PM +0200, Dragan Simic wrote:

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Dragan Simic]

On 2024-10-08 19:37, Mark Brown wrote:
> On Tue, Oct 08, 2024 at 07:32:18PM +0200, Dragan Simic wrote:
> 
>> 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!

Re: [PATCH v3 0/5] Add dev_warn_probe() and improve error handling in Rockchip SPI drivers

[Mark Brown]

On Sun, 29 Sep 2024 11:21:12 +0200, Dragan Simic wrote:
> 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