[PATCH 0/3] driver core: add probe error check helper

[PATCH 0/3] driver core: add probe error check helper

[Andrzej Hajda]

Hi Greg, Rafael,

This patchset proposes probe helper function which should simplify little bit
resource acquisition error handling, it also extend it with adding defer probe
reason to devices_deferred property:

# cat /sys/kernel/debug/devices_deferred
exynos-hdmi 13970000.hdmi: cannot find out bridge
arizona-extcon
tm2-audio sound: Unable to get codec_dai_name

The last patch was generated by cocci script to show the gain for the most
obvious probe_err usage scenarios. Of course there are many more places where it
could be used.
If the 1st patch will be accepted I can split 3rd one per subsystems if neccessary.

Regards
Andrzej


Andrzej Hajda (3):
  driver core: add probe_err log helper
  driver core: add deferring probe reason to devices_deferred property
  drivers: use probe_err function in obvious cases

 drivers/ata/libahci_platform.c                |  7 +-
 drivers/base/base.h                           |  3 +
 drivers/base/core.c                           | 40 +++++++++++
 drivers/base/dd.c                             | 34 +++++++++-
 drivers/base/power/domain.c                   | 10 ++-
 drivers/bus/fsl-mc/fsl-mc-bus.c               |  8 +--
 drivers/bus/ts-nbus.c                         |  8 +--
 drivers/clk/qcom/clk-spmi-pmic-div.c          |  9 +--
 drivers/clk/sunxi-ng/ccu-sun8i-de2.c          | 16 ++---
 drivers/clk/sunxi-ng/ccu-sun9i-a80-de.c       | 11 ++--
 drivers/clk/sunxi-ng/ccu-sun9i-a80-usb.c      |  5 +-
 drivers/cpufreq/qcom-cpufreq-kryo.c           | 10 ++-
 drivers/crypto/caam/caamalg_qi2.c             |  3 +-
 drivers/firmware/arm_scmi/driver.c            |  9 +--
 drivers/firmware/imx/imx-scu.c                |  7 +-
 drivers/gpio/gpio-pca953x.c                   |  4 +-
 drivers/gpio/gpio-pisosr.c                    |  9 +--
 .../gpu/drm/omapdrm/displays/encoder-opa362.c |  8 +--
 .../gpu/drm/omapdrm/displays/encoder-tfp410.c |  8 +--
 .../drm/omapdrm/displays/encoder-tpd12s015.c  |  8 +--
 drivers/gpu/drm/omapdrm/dss/dpi.c             |  8 +--
 drivers/gpu/drm/omapdrm/dss/dsi.c             |  8 +--
 drivers/gpu/drm/omapdrm/dss/hdmi4.c           |  8 +--
 drivers/gpu/drm/omapdrm/dss/hdmi5.c           |  8 +--
 drivers/gpu/drm/omapdrm/dss/sdi.c             |  8 +--
 drivers/gpu/drm/omapdrm/dss/venc.c            |  8 +--
 drivers/gpu/drm/panel/panel-lvds.c            | 10 ++-
 drivers/gpu/drm/panel/panel-simple.c          |  5 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c         |  5 +-
 drivers/gpu/drm/vc4/vc4_dsi.c                 | 23 +++----
 drivers/hid/i2c-hid/i2c-hid-core.c            |  9 +--
 drivers/hwmon/pwm-fan.c                       |  7 +-
 drivers/i2c/busses/i2c-bcm2835.c              |  8 +--
 drivers/i2c/busses/i2c-rk3x.c                 | 10 ++-
 drivers/i2c/muxes/i2c-arb-gpio-challenge.c    | 30 +++------
 drivers/i2c/muxes/i2c-mux-gpmux.c             | 16 ++---
 drivers/iio/adc/envelope-detector.c           | 23 +++----
 drivers/iio/adc/rcar-gyroadc.c                |  5 +-
 drivers/iio/afe/iio-rescale.c                 |  8 +--
 drivers/iio/dac/dpot-dac.c                    | 16 ++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 11 ++--
 drivers/iio/multiplexer/iio-mux.c             | 16 ++---
 drivers/input/keyboard/bcm-keypad.c           |  8 +--
 drivers/input/misc/pwm-beeper.c               | 12 ++--
 drivers/input/misc/pwm-vibra.c                | 18 ++---
 drivers/input/mouse/elan_i2c_core.c           |  6 +-
 drivers/input/touchscreen/bu21029_ts.c        | 14 ++--
 drivers/input/touchscreen/chipone_icn8318.c   |  5 +-
 drivers/input/touchscreen/ektf2127.c          |  5 +-
 drivers/input/touchscreen/elants_i2c.c        | 16 ++---
 drivers/input/touchscreen/pixcir_i2c_ts.c     | 10 ++-
 drivers/input/touchscreen/raydium_i2c_ts.c    | 20 +++---
 .../input/touchscreen/resistive-adc-touch.c   |  9 +--
 drivers/input/touchscreen/silead.c            | 15 ++---
 drivers/input/touchscreen/sis_i2c.c           | 12 ++--
 drivers/input/touchscreen/surface3_spi.c      |  9 +--
 drivers/leds/leds-pwm.c                       |  7 +-
 drivers/media/i2c/ad5820.c                    |  9 +--
 drivers/media/i2c/tc358743.c                  | 10 ++-
 drivers/media/platform/omap3isp/isp.c         |  3 +-
 drivers/media/platform/video-mux.c            |  4 +-
 drivers/media/rc/gpio-ir-recv.c               |  5 +-
 drivers/media/rc/gpio-ir-tx.c                 | 10 ++-
 drivers/mfd/madera-core.c                     |  6 +-
 drivers/mmc/host/bcm2835.c                    |  3 +-
 drivers/mmc/host/davinci_mmc.c                |  5 +-
 drivers/mmc/host/dw_mmc-zx.c                  |  6 +-
 drivers/mmc/host/jz4740_mmc.c                 |  5 +-
 drivers/mmc/host/meson-gx-mmc.c               | 17 ++---
 drivers/mmc/host/sdhci-of-arasan.c            |  8 +--
 drivers/mtd/nand/onenand/omap2.c              |  5 +-
 drivers/mtd/nand/raw/atmel/nand-controller.c  | 20 +++---
 drivers/mux/gpio.c                            |  9 +--
 drivers/net/dsa/lantiq_gswip.c                |  8 +--
 drivers/net/ethernet/renesas/sh_eth.c         |  3 +-
 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 10 +--
 .../ethernet/stmicro/stmmac/dwmac-meson8b.c   |  9 +--
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 10 +--
 drivers/net/ethernet/ti/cpsw.c                |  9 ++-
 drivers/net/ieee802154/mcr20a.c               |  5 +-
 drivers/net/phy/mdio-mux-mmioreg.c            |  9 +--
 drivers/opp/core.c                            | 12 ++--
 drivers/pci/controller/pcie-rockchip.c        | 66 +++++++------------
 drivers/phy/amlogic/phy-meson-gxl-usb2.c      |  9 +--
 drivers/phy/amlogic/phy-meson-gxl-usb3.c      |  9 +--
 drivers/phy/lantiq/phy-lantiq-rcu-usb2.c      |  8 +--
 drivers/phy/qualcomm/phy-qcom-qmp.c           | 21 +++---
 drivers/phy/qualcomm/phy-qcom-qusb2.c         | 19 ++----
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c |  8 +--
 drivers/phy/rockchip/phy-rockchip-pcie.c      |  9 +--
 drivers/phy/st/phy-stm32-usbphyc.c            | 10 ++-
 drivers/platform/x86/intel_cht_int33fe.c      |  8 +--
 drivers/power/supply/lego_ev3_battery.c       | 24 +++----
 drivers/pwm/pwm-rockchip.c                    | 11 ++--
 drivers/regulator/gpio-regulator.c            |  7 +-
 drivers/remoteproc/da8xx_remoteproc.c         | 11 ++--
 drivers/remoteproc/qcom_q6v5.c                | 55 ++++++----------
 drivers/remoteproc/qcom_q6v5_adsp.c           |  9 +--
 drivers/remoteproc/qcom_q6v5_mss.c            | 11 ++--
 drivers/remoteproc/qcom_q6v5_pas.c            | 19 ++----
 drivers/remoteproc/qcom_wcnss_iris.c          |  8 +--
 drivers/reset/reset-meson-audio-arb.c         |  8 +--
 drivers/rtc/rtc-rk808.c                       | 10 ++-
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c        |  7 +-
 drivers/soc/amlogic/meson-gx-pwrc-vpu.c       |  8 +--
 drivers/soc/imx/gpcv2.c                       |  9 ++-
 drivers/soc/lantiq/gphy.c                     |  8 +--
 drivers/soc/qcom/rpmh-rsc.c                   |  9 +--
 drivers/soc/qcom/smem.c                       |  8 +--
 drivers/soc/qcom/smp2p.c                      |  9 +--
 drivers/spi/spi-tegra114.c                    |  6 +-
 drivers/spi/spi-tegra20-slink.c               |  6 +-
 .../clocking-wizard/clk-xlnx-clock-wizard.c   | 16 ++---
 drivers/thermal/broadcom/bcm2835_thermal.c    |  5 +-
 drivers/thermal/hisi_thermal.c                |  5 +-
 drivers/tty/serial/8250/8250_dw.c             |  7 +-
 drivers/tty/serial/8250/8250_ingenic.c        | 12 ++--
 drivers/tty/serial/amba-pl011.c               |  7 +-
 drivers/usb/chipidea/ci_hdrc_imx.c            |  5 +-
 drivers/usb/chipidea/ci_hdrc_msm.c            |  3 +-
 drivers/usb/chipidea/ci_hdrc_usb2.c           |  7 +-
 drivers/usb/dwc3/core.c                       | 24 +++----
 drivers/usb/host/ehci-mv.c                    |  3 +-
 drivers/usb/host/ehci-omap.c                  |  5 +-
 drivers/usb/host/ohci-da8xx.c                 |  6 +-
 drivers/usb/musb/da8xx.c                      |  8 +--
 drivers/usb/musb/musb_cppi41.c                |  5 +-
 drivers/usb/typec/tcpm/fusb302.c              |  3 +-
 drivers/video/backlight/gpio_backlight.c      | 12 +---
 drivers/video/backlight/pwm_bl.c              |  3 +-
 drivers/watchdog/davinci_wdt.c                |  8 +--
 include/linux/device.h                        |  2 +
 sound/soc/atmel/tse850-pcm5142.c              | 32 ++++-----
 sound/soc/codecs/es7241.c                     | 13 ++--
 sound/soc/codecs/max9759.c                    | 15 ++---
 sound/soc/codecs/max9860.c                    |  8 +--
 sound/soc/codecs/pcm3168a.c                   |  8 +--
 sound/soc/codecs/sgtl5000.c                   |  5 +-
 sound/soc/codecs/simple-amplifier.c           |  5 +-
 sound/soc/codecs/ssm2305.c                    |  6 +-
 sound/soc/davinci/davinci-mcasp.c             | 11 ++--
 sound/soc/generic/audio-graph-card.c          |  3 +-
 sound/soc/generic/audio-graph-scu-card.c      |  3 +-
 sound/soc/generic/simple-card.c               |  3 +-
 sound/soc/generic/simple-scu-card.c           |  3 +-
 sound/soc/img/img-i2s-in.c                    |  8 +--
 sound/soc/img/img-i2s-out.c                   | 24 +++----
 sound/soc/img/img-parallel-out.c              | 24 +++----
 sound/soc/img/img-spdif-in.c                  |  8 +--
 sound/soc/img/img-spdif-out.c                 | 24 +++----
 sound/soc/img/pistachio-internal-dac.c        |  6 +-
 sound/soc/meson/axg-card.c                    |  7 +-
 sound/soc/meson/axg-fifo.c                    | 20 +++---
 sound/soc/meson/axg-pdm.c                     | 12 +---
 sound/soc/meson/axg-spdifout.c                |  8 +--
 sound/soc/meson/axg-tdm-formatter.c           | 22 ++-----
 sound/soc/meson/axg-tdm-interface.c           |  8 +--
 sound/soc/mxs/mxs-sgtl5000.c                  |  9 +--
 sound/soc/sunxi/sun4i-codec.c                 |  5 +-
 159 files changed, 658 insertions(+), 1031 deletions(-)

-- 
2.18.0

[PATCH 1/3] driver core: add probe_err log helper

[Andrzej Hajda]

During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code seqences with simple call, so code:
	if (err != -EPROBE_DEFER)
		dev_err(dev, ...);
	return err;
becomes:
	return probe_err(dev, err, ...);

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/base/core.c    | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..23fabefb217a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+/**
+ * probe_err - 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 message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * 	if (err != -EPROBE_DEFER)
+ * 		dev_err(dev, ...);
+ * 	return err;
+ * with
+ * 	return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	if (err != -EPROBE_DEFER) {
+		va_start(args, fmt);
+
+		vaf.fmt = fmt;
+		vaf.va = &args;
+
+		__dev_printk(KERN_ERR, dev, &vaf);
+		va_end(args);
+	}
+
+	return err;
+}
+
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
 	return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 90224e75ade4..06c2c797d132 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1577,6 +1577,8 @@ do {									\
 	WARN_ONCE(condition, "%s %s: " format, \
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.18.0

[PATCH 1/3] driver core: add probe_err log helper

[Javier Martinez Canillas]

Hello Andrzej,

On 10/16/2018 09:22 AM, Andrzej Hajda wrote:
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code seqences with simple call, so code:
> 	if (err != -EPROBE_DEFER)
> 		dev_err(dev, ...);
> 	return err;
> becomes:
> 	return probe_err(dev, err, ...);
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

[PATCH 1/3] driver core: add probe_err log helper

[Mark Brown]

On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote:

> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> +	struct va_format vaf;
> +	va_list args;

...

> +	return err;
> +}
> +

This will need an EXPORT_SYMBOL for modules won't it?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181016/57827625/attachment-0001.sig>

[PATCH 1/3] driver core: add probe_err log helper

[Greg Kroah-Hartman]

On Tue, Oct 16, 2018 at 11:27:15AM +0100, Mark Brown wrote:
> On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote:
> 
> > +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> > +{
> > +	struct va_format vaf;
> > +	va_list args;
> 
> ...
> 
> > +	return err;
> > +}
> > +
> 
> This will need an EXPORT_SYMBOL for modules won't it?

EXPORT_SYMBOL_GPL() to be specific, as it is dealing with the driver
core.

thanks,

greg k-h

[PATCH 1/3] driver core: add probe_err log helper

[Andy Shevchenko]

On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code seqences with simple call, so code:
>         if (err != -EPROBE_DEFER)
>                 dev_err(dev, ...);
>         return err;
> becomes:
>         return probe_err(dev, err, ...);
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/base/core.c    | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  2 ++
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 04bbcd779e11..23fabefb217a 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
>  #endif
>
> +/**
> + * probe_err - 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 message if the error is not -EPROBE_DEFER and propagate it.
> + * It replaces code sequence:
> + *     if (err != -EPROBE_DEFER)
> + *             dev_err(dev, ...);
> + *     return err;
> + * with
> + *     return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> +       struct va_format vaf;
> +       va_list args;
> +

> +       if (err != -EPROBE_DEFER) {

Why not

if (err == ...)
 return err;

...
return err;

?

Better to read, better to maintain. No?

> +               va_start(args, fmt);
> +
> +               vaf.fmt = fmt;
> +               vaf.va = &args;
> +

> +               __dev_printk(KERN_ERR, dev, &vaf);

It would be nice to print an error code as well, wouldn't it?

> +               va_end(args);
> +       }
> +
> +       return err;
> +}
> +
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
>         return fwnode && !IS_ERR(fwnode->secondary);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 90224e75ade4..06c2c797d132 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1577,6 +1577,8 @@ do {                                                                      \
>         WARN_ONCE(condition, "%s %s: " format, \
>                         dev_driver_string(dev), dev_name(dev), ## arg)
>
> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>         MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
> --
> 2.18.0
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH 1/3] driver core: add probe_err log helper

[Andrzej Hajda]

On 16.10.2018 13:01, Andy Shevchenko wrote:
> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> During probe every time driver gets resource it should usually check for error
>> printk some message if it is not -EPROBE_DEFER and return the error. This
>> pattern is simple but requires adding few lines after any resource acquisition
>> code, as a result it is often omited or implemented only partially.
>> probe_err helps to replace such code seqences with simple call, so code:
>>         if (err != -EPROBE_DEFER)
>>                 dev_err(dev, ...);
>>         return err;
>> becomes:
>>         return probe_err(dev, err, ...);
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/base/core.c    | 37 +++++++++++++++++++++++++++++++++++++
>>  include/linux/device.h |  2 ++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 04bbcd779e11..23fabefb217a 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>
>>  #endif
>>
>> +/**
>> + * probe_err - 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 message if the error is not -EPROBE_DEFER and propagate it.
>> + * It replaces code sequence:
>> + *     if (err != -EPROBE_DEFER)
>> + *             dev_err(dev, ...);
>> + *     return err;
>> + * with
>> + *     return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>> +{
>> +       struct va_format vaf;
>> +       va_list args;
>> +
>> +       if (err != -EPROBE_DEFER) {
> Why not
>
> if (err == ...)
>  return err;
>
> ...
> return err;
>
> ?
>
> Better to read, better to maintain. No?

Yes, anyway next patch will re-factor it anyway.

>
>> +               va_start(args, fmt);
>> +
>> +               vaf.fmt = fmt;
>> +               vaf.va = &args;
>> +
>> +               __dev_printk(KERN_ERR, dev, &vaf);
> It would be nice to print an error code as well, wouldn't it?

Hmm, on probe fail error is printed anyway (with exception of
EPROBE_DEFER, ENODEV and ENXIO):
??? "probe of %s failed with error %d\n"
On the other side currently some drivers prints the error code anyway
via dev_err or similar, so I guess during conversion to probe_err it
should be removed then.

If we add error code to probe_err is it OK to report it this way?
??? dev_err(dev, "%V, %d\n", &vaf, err);

Regards
Andrzej

>
>> +               va_end(args);
>> +       }
>> +
>> +       return err;
>> +}
>> +
>>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>  {
>>         return fwnode && !IS_ERR(fwnode->secondary);
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 90224e75ade4..06c2c797d132 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -1577,6 +1577,8 @@ do {                                                                      \
>>         WARN_ONCE(condition, "%s %s: " format, \
>>                         dev_driver_string(dev), dev_name(dev), ## arg)
>>
>> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>>  /* Create alias, so I can be autoloaded. */
>>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>>         MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
>> --
>> 2.18.0
>>
>

[PATCH 1/3] driver core: add probe_err log helper

[Andrzej Hajda]

On 16.10.2018 13:29, Andrzej Hajda wrote:
> On 16.10.2018 13:01, Andy Shevchenko wrote:
>> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> During probe every time driver gets resource it should usually check for error
>>> printk some message if it is not -EPROBE_DEFER and return the error. This
>>> pattern is simple but requires adding few lines after any resource acquisition
>>> code, as a result it is often omited or implemented only partially.
>>> probe_err helps to replace such code seqences with simple call, so code:
>>>         if (err != -EPROBE_DEFER)
>>>                 dev_err(dev, ...);
>>>         return err;
>>> becomes:
>>>         return probe_err(dev, err, ...);
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/base/core.c    | 37 +++++++++++++++++++++++++++++++++++++
>>>  include/linux/device.h |  2 ++
>>>  2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 04bbcd779e11..23fabefb217a 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>>
>>>  #endif
>>>
>>> +/**
>>> + * probe_err - 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 message if the error is not -EPROBE_DEFER and propagate it.
>>> + * It replaces code sequence:
>>> + *     if (err != -EPROBE_DEFER)
>>> + *             dev_err(dev, ...);
>>> + *     return err;
>>> + * with
>>> + *     return probe_err(dev, err, ...);
>>> + *
>>> + * Returns @err.
>>> + *
>>> + */
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>> +{
>>> +       struct va_format vaf;
>>> +       va_list args;
>>> +
>>> +       if (err != -EPROBE_DEFER) {
>> Why not
>>
>> if (err == ...)
>>  return err;
>>
>> ...
>> return err;
>>
>> ?
>>
>> Better to read, better to maintain. No?
> Yes, anyway next patch will re-factor it anyway.
>
>>> +               va_start(args, fmt);
>>> +
>>> +               vaf.fmt = fmt;
>>> +               vaf.va = &args;
>>> +
>>> +               __dev_printk(KERN_ERR, dev, &vaf);
>> It would be nice to print an error code as well, wouldn't it?
> Hmm, on probe fail error is printed anyway (with exception of
> EPROBE_DEFER, ENODEV and ENXIO):
> ??? "probe of %s failed with error %d\n"
> On the other side currently some drivers prints the error code anyway
> via dev_err or similar, so I guess during conversion to probe_err it
> should be removed then.
>
> If we add error code to probe_err is it OK to report it this way?
> ??? dev_err(dev, "%V, %d\n", &vaf, err);

Ups, I forgot that message passed to probe_err will contain already
newline character.
So the err must be before message passed to probe_err, for example:
??? dev_err(dev, "err=%d: %V\n", err, &vaf);
Is it OK? Or better leave this part of the patch as is?

Regards
Andrzej

>
> Regards
> Andrzej
>
>>> +               va_end(args);
>>> +       }
>>> +
>>> +       return err;
>>> +}
>>> +
>>>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>>  {
>>>         return fwnode && !IS_ERR(fwnode->secondary);
>>> diff --git a/include/linux/device.h b/include/linux/device.h
>>> index 90224e75ade4..06c2c797d132 100644
>>> --- a/include/linux/device.h
>>> +++ b/include/linux/device.h
>>> @@ -1577,6 +1577,8 @@ do {                                                                      \
>>>         WARN_ONCE(condition, "%s %s: " format, \
>>>                         dev_driver_string(dev), dev_name(dev), ## arg)
>>>
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
>>> +
>>>  /* Create alias, so I can be autoloaded. */
>>>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>>>         MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
>>> --
>>> 2.18.0
>>>

[PATCH 1/3] driver core: add probe_err log helper

[Andy Shevchenko]

On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 16.10.2018 13:29, Andrzej Hajda wrote:
> > On 16.10.2018 13:01, Andy Shevchenko wrote:
> >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>> During probe every time driver gets resource it should usually check for error
> >>> printk some message if it is not -EPROBE_DEFER and return the error. This
> >>> pattern is simple but requires adding few lines after any resource acquisition
> >>> code, as a result it is often omited or implemented only partially.
> >>> probe_err helps to replace such code seqences with simple call, so code:
> >>>         if (err != -EPROBE_DEFER)
> >>>                 dev_err(dev, ...);
> >>>         return err;
> >>> becomes:
> >>>         return probe_err(dev, err, ...);

> >>> +               va_start(args, fmt);
> >>> +
> >>> +               vaf.fmt = fmt;
> >>> +               vaf.va = &args;
> >>> +
> >>> +               __dev_printk(KERN_ERR, dev, &vaf);

> >> It would be nice to print an error code as well, wouldn't it?
> > Hmm, on probe fail error is printed anyway (with exception of
> > EPROBE_DEFER, ENODEV and ENXIO):
> >     "probe of %s failed with error %d\n"
> > On the other side currently some drivers prints the error code anyway
> > via dev_err or similar, so I guess during conversion to probe_err it
> > should be removed then.
> >
> > If we add error code to probe_err is it OK to report it this way?
> >     dev_err(dev, "%V, %d\n", &vaf, err);
>
> Ups, I forgot that message passed to probe_err will contain already
> newline character.

You may consider not to pass it.

> So the err must be before message passed to probe_err, for example:
>     dev_err(dev, "err=%d: %V\n", err, &vaf);

> Is it OK?

For me would work either (no \n in the message, or err preceding the message).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/3] driver core: add probe_err log helper

[Andrzej Hajda]

During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code sequences with simple call, so code:
	if (err != -EPROBE_DEFER)
		dev_err(dev, ...);
	return err;
becomes:
	return probe_err(dev, err, ...);

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

---
v2:
  - added error value to log message,
  - fixed code style,
  - added EXPORT_SYMBOL_GPL,
  - Added R-B by Javier (I hope the changes did not invalidate it).
---
 drivers/base/core.c    | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..aa6f3229d066 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string, not ended with newline
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * 	if (err != -EPROBE_DEFER)
+ * 		dev_err(dev, ...);
+ * 	return err;
+ * with
+ * 	return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	if (err == -EPROBE_DEFER)
+		return err;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+	dev_err(dev, "%pV, %d\n", &vaf, err);
+	va_end(args);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(probe_err);
+
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
 	return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 90224e75ade4..06c2c797d132 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1577,6 +1577,8 @@ do {									\
 	WARN_ONCE(condition, "%s %s: " format, \
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.18.0

[PATCH v2 1/3] driver core: add probe_err log helper

[Mark Brown]

On Wed, Oct 17, 2018 at 10:58:24AM +0200, Andrzej Hajda wrote:
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code sequences with simple call, so code:

Reviwed-by: Mark Brown <broonie@kernel.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181017/c8603fbe/attachment.sig>

[PATCH v2 1/3] driver core: add probe_err log helper

[Andy Shevchenko]

On Wed, Oct 17, 2018 at 11:58 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code sequences with simple call, so code:
>         if (err != -EPROBE_DEFER)
>                 dev_err(dev, ...);
>         return err;
> becomes:
>         return probe_err(dev, err, ...);
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

Looks good to me,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> ---
> v2:
>   - added error value to log message,
>   - fixed code style,
>   - added EXPORT_SYMBOL_GPL,
>   - Added R-B by Javier (I hope the changes did not invalidate it).
> ---
>  drivers/base/core.c    | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  2 ++
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 04bbcd779e11..aa6f3229d066 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
>  #endif
>
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test
> + * @fmt: printf-style format string, not ended with newline
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate it.
> + * It replaces code sequence:
> + *     if (err != -EPROBE_DEFER)
> + *             dev_err(dev, ...);
> + *     return err;
> + * with
> + *     return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> +       struct va_format vaf;
> +       va_list args;
> +
> +       if (err == -EPROBE_DEFER)
> +               return err;
> +
> +       va_start(args, fmt);
> +       vaf.fmt = fmt;
> +       vaf.va = &args;
> +       dev_err(dev, "%pV, %d\n", &vaf, err);
> +       va_end(args);
> +
> +       return err;
> +}
> +EXPORT_SYMBOL_GPL(probe_err);
> +
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
>         return fwnode && !IS_ERR(fwnode->secondary);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 90224e75ade4..06c2c797d132 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1577,6 +1577,8 @@ do {                                                                      \
>         WARN_ONCE(condition, "%s %s: " format, \
>                         dev_driver_string(dev), dev_name(dev), ## arg)
>
> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>         MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
> --
> 2.18.0
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/3] driver core: add probe_err log helper

[Greg Kroah-Hartman]

On Wed, Oct 17, 2018 at 10:58:24AM +0200, Andrzej Hajda wrote:
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code sequences with simple call, so code:
> 	if (err != -EPROBE_DEFER)
> 		dev_err(dev, ...);
> 	return err;
> becomes:
> 	return probe_err(dev, err, ...);
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> ---
> v2:
>   - added error value to log message,
>   - fixed code style,
>   - added EXPORT_SYMBOL_GPL,
>   - Added R-B by Javier (I hope the changes did not invalidate it).
> ---
>  drivers/base/core.c    | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  2 ++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 04bbcd779e11..aa6f3229d066 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>  
>  #endif
>  
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test
> + * @fmt: printf-style format string, not ended with newline
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate it.
> + * It replaces code sequence:
> + * 	if (err != -EPROBE_DEFER)
> + * 		dev_err(dev, ...);
> + * 	return err;
> + * with
> + * 	return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> +	struct va_format vaf;
> +	va_list args;
> +
> +	if (err == -EPROBE_DEFER)
> +		return err;
> +
> +	va_start(args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +	dev_err(dev, "%pV, %d\n", &vaf, err);
> +	va_end(args);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(probe_err);
> +
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
>  	return fwnode && !IS_ERR(fwnode->secondary);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 90224e75ade4..06c2c797d132 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1577,6 +1577,8 @@ do {									\
>  	WARN_ONCE(condition, "%s %s: " format, \
>  			dev_driver_string(dev), dev_name(dev), ## arg)
>  
> +int probe_err(const struct device *dev, int err, const char *fmt, ...);

__printf(3, 4) ?

[PATCH 1/3] driver core: add probe_err log helper

[Russell King - ARM Linux]

On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > >>> During probe every time driver gets resource it should usually check for error
> > >>> printk some message if it is not -EPROBE_DEFER and return the error. This
> > >>> pattern is simple but requires adding few lines after any resource acquisition
> > >>> code, as a result it is often omited or implemented only partially.
> > >>> probe_err helps to replace such code seqences with simple call, so code:
> > >>>         if (err != -EPROBE_DEFER)
> > >>>                 dev_err(dev, ...);
> > >>>         return err;
> > >>> becomes:
> > >>>         return probe_err(dev, err, ...);
> 
> > >>> +               va_start(args, fmt);
> > >>> +
> > >>> +               vaf.fmt = fmt;
> > >>> +               vaf.va = &args;
> > >>> +
> > >>> +               __dev_printk(KERN_ERR, dev, &vaf);
> 
> > >> It would be nice to print an error code as well, wouldn't it?
> > > Hmm, on probe fail error is printed anyway (with exception of
> > > EPROBE_DEFER, ENODEV and ENXIO):
> > >     "probe of %s failed with error %d\n"
> > > On the other side currently some drivers prints the error code anyway
> > > via dev_err or similar, so I guess during conversion to probe_err it
> > > should be removed then.
> > >
> > > If we add error code to probe_err is it OK to report it this way?
> > >     dev_err(dev, "%V, %d\n", &vaf, err);
> >
> > Ups, I forgot that message passed to probe_err will contain already
> > newline character.
> 
> You may consider not to pass it.

It's normal to pass the '\n', so by doing this, we create the situation
where this function becomes the exception to the norm.  That's not a
good idea - we will see people forget that appending '\n' should not
be done for this particular function.

While we could add a checkpatch rule, that's hassle (extra rework).  In
any case, I think the message would be much better formatted if we did:

	dev_err(dev, "error %d: %V", err, &vaf);

which means we end up with (eg):

	error -5: request_irq failed for irq 9

rather than:

	request_irq failed for irq 9, -5

which is more confusing.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 1/3] driver core: add probe_err log helper

[Andy Shevchenko]

On Wed, Oct 17, 2018 at 2:29 PM Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
>
> On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > > >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > > >>> During probe every time driver gets resource it should usually check for error
> > > >>> printk some message if it is not -EPROBE_DEFER and return the error. This
> > > >>> pattern is simple but requires adding few lines after any resource acquisition
> > > >>> code, as a result it is often omited or implemented only partially.
> > > >>> probe_err helps to replace such code seqences with simple call, so code:
> > > >>>         if (err != -EPROBE_DEFER)
> > > >>>                 dev_err(dev, ...);
> > > >>>         return err;
> > > >>> becomes:
> > > >>>         return probe_err(dev, err, ...);
> >
> > > >>> +               va_start(args, fmt);
> > > >>> +
> > > >>> +               vaf.fmt = fmt;
> > > >>> +               vaf.va = &args;
> > > >>> +
> > > >>> +               __dev_printk(KERN_ERR, dev, &vaf);
> >
> > > >> It would be nice to print an error code as well, wouldn't it?
> > > > Hmm, on probe fail error is printed anyway (with exception of
> > > > EPROBE_DEFER, ENODEV and ENXIO):
> > > >     "probe of %s failed with error %d\n"
> > > > On the other side currently some drivers prints the error code anyway
> > > > via dev_err or similar, so I guess during conversion to probe_err it
> > > > should be removed then.
> > > >
> > > > If we add error code to probe_err is it OK to report it this way?
> > > >     dev_err(dev, "%V, %d\n", &vaf, err);
> > >
> > > Ups, I forgot that message passed to probe_err will contain already
> > > newline character.
> >
> > You may consider not to pass it.
>
> It's normal to pass the '\n', so by doing this, we create the situation
> where this function becomes the exception to the norm.  That's not a
> good idea - we will see people forget that appending '\n' should not
> be done for this particular function.
>
> While we could add a checkpatch rule, that's hassle (extra rework).  In
> any case, I think the message would be much better formatted if we did:
>
>         dev_err(dev, "error %d: %V", err, &vaf);
>
> which means we end up with (eg):
>
>         error -5: request_irq failed for irq 9
>
> rather than:
>
>         request_irq failed for irq 9, -5
>
> which is more confusing.

As I said earlier, I'm fine to either variant and I see your point
here, so, I tend to agree to this variant.

P.S. Andrzej, in any case my Rb tag, which I just gave, stays.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 1/4] driver core: add probe_err log helper

[Andrzej Hajda]

During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code sequences with simple call, so code:
	if (err != -EPROBE_DEFER)
		dev_err(dev, ...);
	return err;
becomes:
	return probe_err(dev, err, ...);

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v3:
  - added 'extern __printf(3, 4)' decorators to probe_err,
  - changed error logging format - newline at the end,
  - added empty lines in probe_err around dev_err,
  - added R-b by Mark and Andy.
v2:
  - added error value to log message,
  - fixed code style,
  - added EXPORT_SYMBOL_GPL,
  - Added R-B by Javier (I hope the changes did not invalidate it).
---
 drivers/base/core.c    | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..1f3e99c2ef03 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3067,6 +3067,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+/**
+ * probe_err - 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 message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * 	if (err != -EPROBE_DEFER)
+ * 		dev_err(dev, ...);
+ * 	return err;
+ * with
+ * 	return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	if (err == -EPROBE_DEFER)
+		return err;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	dev_err(dev, "error %d: %pV", err, &vaf);
+
+	va_end(args);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(probe_err);
+
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
 	return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 90224e75ade4..6831677d5278 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1577,6 +1577,9 @@ do {									\
 	WARN_ONCE(condition, "%s %s: " format, \
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
+extern __printf(3, 4)
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.18.0

[PATCH 1/3] driver core: add probe_err log helper

[Joe Perches]

On Wed, 2018-10-17 at 12:29 +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > > > > On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > > > > > During probe every time driver gets resource it should usually check for error
> > > > > > printk some message if it is not -EPROBE_DEFER and return the error. This
> > > > > > pattern is simple but requires adding few lines after any resource acquisition
> > > > > > code, as a result it is often omited or implemented only partially.
> > > > > > probe_err helps to replace such code seqences with simple call, so code:
> > > > > >         if (err != -EPROBE_DEFER)
> > > > > >                 dev_err(dev, ...);
> > > > > >         return err;
> > > > > > becomes:
> > > > > >         return probe_err(dev, err, ...);
> > > > > > +               va_start(args, fmt);
> > > > > > +
> > > > > > +               vaf.fmt = fmt;
> > > > > > +               vaf.va = &args;
> > > > > > +
> > > > > > +               __dev_printk(KERN_ERR, dev, &vaf);
> > > > > It would be nice to print an error code as well, wouldn't it?
> > > > Hmm, on probe fail error is printed anyway (with exception of
> > > > EPROBE_DEFER, ENODEV and ENXIO):
> > > >     "probe of %s failed with error %d\n"
> > > > On the other side currently some drivers prints the error code anyway
> > > > via dev_err or similar, so I guess during conversion to probe_err it
> > > > should be removed then.
> > > > 
> > > > If we add error code to probe_err is it OK to report it this way?
> > > >     dev_err(dev, "%V, %d\n", &vaf, err);
> > > 
> > > Ups, I forgot that message passed to probe_err will contain already
> > > newline character.
> > 
> > You may consider not to pass it.
> 
> It's normal to pass the '\n', so by doing this, we create the situation
> where this function becomes the exception to the norm.  That's not a
> good idea - we will see people forget that appending '\n' should not
> be done for this particular function.
> 
> While we could add a checkpatch rule, that's hassle (extra rework).

It would not be a simple checkpatch rule with high confidence
because of pr_cont uses that may not be in the patch context.

> In I think the message would be much better formatted if we did:
> 
> 	dev_err(dev, "error %d: %V", err, &vaf);

s/%V/%pV/

[PATCH 2/3] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is probe_err function introduced recently,
ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
will be attached to deferred device and printed when user read devices_deferred
property.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c | 15 +++++++++------
 drivers/base/dd.c   | 34 +++++++++++++++++++++++++++++++++-
 3 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 7a419a7a6235..6f9371bfe40b 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -75,6 +75,7 @@ struct device_private {
 	struct klist_node knode_driver;
 	struct klist_node knode_bus;
 	struct list_head deferred_probe;
+	char *deferred_probe_msg;
 	struct device *device;
 };
 #define to_device_private_parent(obj)	\
@@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
 extern void driver_deferred_probe_del(struct device *dev);
+extern void __deferred_probe_set_msg(const struct device *dev, const char *fmt,
+				     va_list args);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 23fabefb217a..df9895adf11b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  *
  * This helper implements common pattern present in probe functions for error
  * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason.
  * It replaces code sequence:
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
@@ -3091,15 +3092,17 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
 	struct va_format vaf;
 	va_list args;
 
-	if (err != -EPROBE_DEFER) {
-		va_start(args, fmt);
+	va_start(args, fmt);
 
-		vaf.fmt = fmt;
-		vaf.va = &args;
+	vaf.fmt = fmt;
+	vaf.va = &args;
 
+	if (err != -EPROBE_DEFER)
 		__dev_printk(KERN_ERR, dev, &vaf);
-		va_end(args);
-	}
+	else
+		__deferred_probe_set_msg(dev, fmt, args);
+
+	va_end(args);
 
 	return err;
 }
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 169412ee4ae8..e2f81e538d4b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/slab.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
 	if (!list_empty(&dev->p->deferred_probe)) {
 		dev_dbg(dev, "Removed from deferred list\n");
 		list_del_init(&dev->p->deferred_probe);
+		kfree(dev->p->deferred_probe_msg);
+		dev->p->deferred_probe_msg = NULL;
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
@@ -202,6 +205,32 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/*
+ * __deferred_probe_set_msg() - Set defer probe reason message for device
+ */
+void __deferred_probe_set_msg(const struct device *dev, const char *fmt,
+				     va_list args)
+{
+	const int size = 128;
+	char **p;
+	int n;
+
+	mutex_lock(&deferred_probe_mutex);
+
+	p = &dev->p->deferred_probe_msg;
+	if (!*p) {
+		*p = kmalloc(size, GFP_KERNEL);
+		if (!*p)
+			goto end;
+	}
+	n = snprintf(*p, size, "%s %s: ", dev_driver_string(dev), dev_name(dev));
+	if (n < size)
+		vsnprintf(*p + n, size - n, fmt, args);
+
+end:
+	mutex_unlock(&deferred_probe_mutex);
+}
+
 /*
  * deferred_devs_show() - Show the devices in the deferred probe pending list.
  */
@@ -212,7 +241,10 @@ static int deferred_devs_show(struct seq_file *s, void *data)
 	mutex_lock(&deferred_probe_mutex);
 
 	list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
-		seq_printf(s, "%s\n", dev_name(curr->device));
+		if (curr->device->p->deferred_probe_msg)
+			seq_puts(s, curr->device->p->deferred_probe_msg);
+		else
+			seq_printf(s, "%s\n", dev_name(curr->device));
 
 	mutex_unlock(&deferred_probe_mutex);
 
-- 
2.18.0

[PATCH 2/3] driver core: add deferring probe reason to devices_deferred property

[Javier Martinez Canillas]

On 10/16/2018 09:22 AM, Andrzej Hajda wrote:
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
> will be attached to deferred device and printed when user read devices_deferred
> property.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c | 15 +++++++++------
>  drivers/base/dd.c   | 34 +++++++++++++++++++++++++++++++++-
>  3 files changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 7a419a7a6235..6f9371bfe40b 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -75,6 +75,7 @@ struct device_private {
>  	struct klist_node knode_driver;
>  	struct klist_node knode_bus;
>  	struct list_head deferred_probe;
> +	char *deferred_probe_msg;
>  	struct device *device;
>  };
>  #define to_device_private_parent(obj)	\
> @@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
>  extern void driver_detach(struct device_driver *drv);
>  extern int driver_probe_device(struct device_driver *drv, struct device *dev);
>  extern void driver_deferred_probe_del(struct device *dev);
> +extern void __deferred_probe_set_msg(const struct device *dev, const char *fmt,
> +				     va_list args);
>  static inline int driver_match_device(struct device_driver *drv,
>  				      struct device *dev)
>  {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 23fabefb217a..df9895adf11b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>   *
>   * This helper implements common pattern present in probe functions for error
>   * checking: print message if the error is not -EPROBE_DEFER and propagate it.
> + * In case of -EPROBE_DEFER it sets defer probe reason.
>   * It replaces code sequence:
>   * 	if (err != -EPROBE_DEFER)
>   * 		dev_err(dev, ...);
> @@ -3091,15 +3092,17 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>  	struct va_format vaf;
>  	va_list args;
>  
> -	if (err != -EPROBE_DEFER) {
> -		va_start(args, fmt);
> +	va_start(args, fmt);
>  
> -		vaf.fmt = fmt;
> -		vaf.va = &args;
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
>  
> +	if (err != -EPROBE_DEFER)
>  		__dev_printk(KERN_ERR, dev, &vaf);
> -		va_end(args);
> -	}
> +	else
> +		__deferred_probe_set_msg(dev, fmt, args);
> +
> +	va_end(args);
>  
>  	return err;
>  }
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..e2f81e538d4b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/slab.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
>  	if (!list_empty(&dev->p->deferred_probe)) {
>  		dev_dbg(dev, "Removed from deferred list\n");
>  		list_del_init(&dev->p->deferred_probe);
> +		kfree(dev->p->deferred_probe_msg);
> +		dev->p->deferred_probe_msg = NULL;
>  	}
>  	mutex_unlock(&deferred_probe_mutex);
>  }
> @@ -202,6 +205,32 @@ void device_unblock_probing(void)
>  	driver_deferred_probe_trigger();
>  }
>  
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> + */
> +void __deferred_probe_set_msg(const struct device *dev, const char *fmt,
> +				     va_list args)
> +{
> +	const int size = 128;
> +	char **p;
> +	int n;
> +
> +	mutex_lock(&deferred_probe_mutex);
> +
> +	p = &dev->p->deferred_probe_msg;
> +	if (!*p) {
> +		*p = kmalloc(size, GFP_KERNEL);
> +		if (!*p)
> +			goto end;
> +	}
> +	n = snprintf(*p, size, "%s %s: ", dev_driver_string(dev), dev_name(dev));
> +	if (n < size)
> +		vsnprintf(*p + n, size - n, fmt, args);

I wonder if the vsnprintf() return value should be checked and print a warning
if the message was truncated. Probably 128 is enough for most error messages?

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

[PATCH 2/3] driver core: add deferring probe reason to devices_deferred property

[Andy Shevchenko]

On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
> will be attached to deferred device and printed when user read devices_deferred
> property.


> -       if (err != -EPROBE_DEFER) {
> -               va_start(args, fmt);
> +       va_start(args, fmt);
>
> -               vaf.fmt = fmt;
> -               vaf.va = &args;
> +       vaf.fmt = fmt;
> +       vaf.va = &args;
>
> +       if (err != -EPROBE_DEFER)
>                 __dev_printk(KERN_ERR, dev, &vaf);
> -               va_end(args);
> -       }
> +       else
> +               __deferred_probe_set_msg(dev, fmt, args);
> +
> +       va_end(args);

Yeah, ping-pong style of programming (one patch "amends" something
which had been introduced by another).
That's why better to follow my comment for the patch 1.

> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> + */
> +void __deferred_probe_set_msg(const struct device *dev, const char *fmt,
> +                                    va_list args)
> +{
> +       const int size = 128;
> +       char **p;
> +       int n;
> +
> +       mutex_lock(&deferred_probe_mutex);
> +


> +       p = &dev->p->deferred_probe_msg;
> +       if (!*p) {
> +               *p = kmalloc(size, GFP_KERNEL);
> +               if (!*p)
> +                       goto end;
> +       }
> +       n = snprintf(*p, size, "%s %s: ", dev_driver_string(dev), dev_name(dev));
> +       if (n < size)
> +               vsnprintf(*p + n, size - n, fmt, args);

Perhaps kasprintf() instead of this huge plays around the size and allocation?

>         list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
> -               seq_printf(s, "%s\n", dev_name(curr->device));
> +               if (curr->device->p->deferred_probe_msg)
> +                       seq_puts(s, curr->device->p->deferred_probe_msg);
> +               else
> +                       seq_printf(s, "%s\n", dev_name(curr->device));

I wouldn't care (much) about debugfs, though better to keep some order
here, i.e. always print device name.

Something like

seq_printf(s, "%s\t%s\n", dev_name(), deferred_probe_msg ?: "");

and remove that part from above __deferred_probe_set_msg().

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/3] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is probe_err function introduced recently,
ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
will be attached to deferred device and printed when user read devices_deferred
property.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
v2:
  - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
  - use kasprintf instead of bunch of code,
  - keep consistent format of devices_deferred lines,
  - added R-Bs (again I hope changes above are not against it).
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c | 11 +++++++----
 drivers/base/dd.c   | 21 ++++++++++++++++++++-
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 7a419a7a6235..effbd5e7f9f1 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -75,6 +75,7 @@ struct device_private {
 	struct klist_node knode_driver;
 	struct klist_node knode_bus;
 	struct list_head deferred_probe;
+	char *deferred_probe_msg;
 	struct device *device;
 };
 #define to_device_private_parent(obj)	\
@@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
 extern void driver_deferred_probe_del(struct device *dev);
+extern void __deferred_probe_set_msg(const struct device *dev,
+				     struct va_format *vaf);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index aa6f3229d066..560758b2ae79 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  *
  * This helper implements common pattern present in probe functions for error
  * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason.
  * It replaces code sequence:
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
@@ -3091,13 +3092,15 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
 	struct va_format vaf;
 	va_list args;
 
-	if (err == -EPROBE_DEFER)
-		return err;
-
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
-	dev_err(dev, "%pV, %d\n", &vaf, err);
+
+	if (err != -EPROBE_DEFER)
+		dev_err(dev, "%pV, %d\n", &vaf, err);
+	else
+		__deferred_probe_set_msg(dev, &vaf);
+
 	va_end(args);
 
 	return err;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 169412ee4ae8..345fbfe335d1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/slab.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
 	if (!list_empty(&dev->p->deferred_probe)) {
 		dev_dbg(dev, "Removed from deferred list\n");
 		list_del_init(&dev->p->deferred_probe);
+		kfree(dev->p->deferred_probe_msg);
+		dev->p->deferred_probe_msg = NULL;
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
@@ -202,6 +205,21 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/*
+ * __deferred_probe_set_msg() - Set defer probe reason message for device
+ */
+void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
+{
+	mutex_lock(&deferred_probe_mutex);
+
+	if (dev->p->deferred_probe_msg)
+		kfree(dev->p->deferred_probe_msg);
+	dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV",
+					       dev_driver_string(dev), vaf);
+
+	mutex_unlock(&deferred_probe_mutex);
+}
+
 /*
  * deferred_devs_show() - Show the devices in the deferred probe pending list.
  */
@@ -212,7 +230,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
 	mutex_lock(&deferred_probe_mutex);
 
 	list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
-		seq_printf(s, "%s\n", dev_name(curr->device));
+		seq_printf(s, "%s\t%s\n", dev_name(curr->device),
+			   curr->device->p->deferred_probe_msg ?: "");
 
 	mutex_unlock(&deferred_probe_mutex);
 
-- 
2.18.0

[PATCH v2 2/3] driver core: add deferring probe reason to devices_deferred property

[Andy Shevchenko]

On Wed, Oct 17, 2018 at 11:59 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
> will be attached to deferred device and printed when user read devices_deferred
> property.
>

Looks good to me (with one comment below since patch 1 will be changed anyway),
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> v2:
>   - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
>   - use kasprintf instead of bunch of code,
>   - keep consistent format of devices_deferred lines,
>   - added R-Bs (again I hope changes above are not against it).
> ---
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c | 11 +++++++----
>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>  3 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 7a419a7a6235..effbd5e7f9f1 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -75,6 +75,7 @@ struct device_private {
>         struct klist_node knode_driver;
>         struct klist_node knode_bus;
>         struct list_head deferred_probe;
> +       char *deferred_probe_msg;
>         struct device *device;
>  };
>  #define to_device_private_parent(obj)  \
> @@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
>  extern void driver_detach(struct device_driver *drv);
>  extern int driver_probe_device(struct device_driver *drv, struct device *dev);
>  extern void driver_deferred_probe_del(struct device *dev);
> +extern void __deferred_probe_set_msg(const struct device *dev,
> +                                    struct va_format *vaf);
>  static inline int driver_match_device(struct device_driver *drv,
>                                       struct device *dev)
>  {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index aa6f3229d066..560758b2ae79 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>   *
>   * This helper implements common pattern present in probe functions for error
>   * checking: print message if the error is not -EPROBE_DEFER and propagate it.
> + * In case of -EPROBE_DEFER it sets defer probe reason.
>   * It replaces code sequence:
>   *     if (err != -EPROBE_DEFER)
>   *             dev_err(dev, ...);
> @@ -3091,13 +3092,15 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>         struct va_format vaf;
>         va_list args;
>
> -       if (err == -EPROBE_DEFER)
> -               return err;
> -
>         va_start(args, fmt);
>         vaf.fmt = fmt;
>         vaf.va = &args;

> -       dev_err(dev, "%pV, %d\n", &vaf, err);
> +

This new line can be part of patch 1.

> +       if (err != -EPROBE_DEFER)
> +               dev_err(dev, "%pV, %d\n", &vaf, err);
> +       else
> +               __deferred_probe_set_msg(dev, &vaf);
> +

Here you may still keep the same, i.e. positive, conditional.

>         va_end(args);
>
>         return err;
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..345fbfe335d1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/slab.h>
>
>  #include "base.h"
>  #include "power/power.h"
> @@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
>         if (!list_empty(&dev->p->deferred_probe)) {
>                 dev_dbg(dev, "Removed from deferred list\n");
>                 list_del_init(&dev->p->deferred_probe);
> +               kfree(dev->p->deferred_probe_msg);
> +               dev->p->deferred_probe_msg = NULL;
>         }
>         mutex_unlock(&deferred_probe_mutex);
>  }
> @@ -202,6 +205,21 @@ void device_unblock_probing(void)
>         driver_deferred_probe_trigger();
>  }
>
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> + */
> +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
> +{
> +       mutex_lock(&deferred_probe_mutex);
> +
> +       if (dev->p->deferred_probe_msg)
> +               kfree(dev->p->deferred_probe_msg);
> +       dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV",
> +                                              dev_driver_string(dev), vaf);
> +
> +       mutex_unlock(&deferred_probe_mutex);
> +}
> +
>  /*
>   * deferred_devs_show() - Show the devices in the deferred probe pending list.
>   */
> @@ -212,7 +230,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
>         mutex_lock(&deferred_probe_mutex);
>
>         list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
> -               seq_printf(s, "%s\n", dev_name(curr->device));
> +               seq_printf(s, "%s\t%s\n", dev_name(curr->device),
> +                          curr->device->p->deferred_probe_msg ?: "");
>
>         mutex_unlock(&deferred_probe_mutex);
>
> --
> 2.18.0
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/4] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is probe_err function introduced recently,
ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
will be attached to deferred device and printed when user read devices_deferred
property.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v3:
  - adjusted deferred_devs_show, to accept newline ended messages,
  - changed conditonal check to positive,
  - added R-b by Andy.
v2:
  - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
  - use kasprintf instead of bunch of code,
  - keep consistent format of devices_deferred lines,
  - added R-Bs (again I hope changes above are not against it).
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c |  9 +++++----
 drivers/base/dd.c   | 21 ++++++++++++++++++++-
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 7a419a7a6235..effbd5e7f9f1 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -75,6 +75,7 @@ struct device_private {
 	struct klist_node knode_driver;
 	struct klist_node knode_bus;
 	struct list_head deferred_probe;
+	char *deferred_probe_msg;
 	struct device *device;
 };
 #define to_device_private_parent(obj)	\
@@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
 extern void driver_deferred_probe_del(struct device *dev);
+extern void __deferred_probe_set_msg(const struct device *dev,
+				     struct va_format *vaf);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 1f3e99c2ef03..27990110fb24 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  *
  * This helper implements common pattern present in probe functions for error
  * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason.
  * It replaces code sequence:
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
@@ -3091,14 +3092,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
 	struct va_format vaf;
 	va_list args;
 
-	if (err == -EPROBE_DEFER)
-		return err;
-
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	dev_err(dev, "error %d: %pV", err, &vaf);
+	if (err == -EPROBE_DEFER)
+		__deferred_probe_set_msg(dev, &vaf);
+	else
+		dev_err(dev, "error %d: %pV", err, &vaf);
 
 	va_end(args);
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 169412ee4ae8..6e488146b328 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/slab.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
 	if (!list_empty(&dev->p->deferred_probe)) {
 		dev_dbg(dev, "Removed from deferred list\n");
 		list_del_init(&dev->p->deferred_probe);
+		kfree(dev->p->deferred_probe_msg);
+		dev->p->deferred_probe_msg = NULL;
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
@@ -202,6 +205,21 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/*
+ * __deferred_probe_set_msg() - Set defer probe reason message for device
+ */
+void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
+{
+	mutex_lock(&deferred_probe_mutex);
+
+	if (dev->p->deferred_probe_msg)
+		kfree(dev->p->deferred_probe_msg);
+	dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV",
+					       dev_driver_string(dev), vaf);
+
+	mutex_unlock(&deferred_probe_mutex);
+}
+
 /*
  * deferred_devs_show() - Show the devices in the deferred probe pending list.
  */
@@ -212,7 +230,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
 	mutex_lock(&deferred_probe_mutex);
 
 	list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
-		seq_printf(s, "%s\n", dev_name(curr->device));
+		seq_printf(s, "%s\t%s", dev_name(curr->device),
+			   curr->device->p->deferred_probe_msg ?: "\n");
 
 	mutex_unlock(&deferred_probe_mutex);
 
-- 
2.18.0

[PATCH v3 2/4] driver core: add deferring probe reason to devices_deferred property

[Andy Shevchenko]

On Wed, Oct 17, 2018 at 4:24 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
> will be attached to deferred device and printed when user read devices_deferred
> property.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> v3:
>   - adjusted deferred_devs_show, to accept newline ended messages,
>   - changed conditonal check to positive,
>   - added R-b by Andy.
> v2:
>   - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
>   - use kasprintf instead of bunch of code,
>   - keep consistent format of devices_deferred lines,
>   - added R-Bs (again I hope changes above are not against it).
> ---
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c |  9 +++++----
>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>  3 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 7a419a7a6235..effbd5e7f9f1 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -75,6 +75,7 @@ struct device_private {
>         struct klist_node knode_driver;
>         struct klist_node knode_bus;
>         struct list_head deferred_probe;
> +       char *deferred_probe_msg;
>         struct device *device;
>  };
>  #define to_device_private_parent(obj)  \
> @@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
>  extern void driver_detach(struct device_driver *drv);
>  extern int driver_probe_device(struct device_driver *drv, struct device *dev);
>  extern void driver_deferred_probe_del(struct device *dev);
> +extern void __deferred_probe_set_msg(const struct device *dev,
> +                                    struct va_format *vaf);
>  static inline int driver_match_device(struct device_driver *drv,
>                                       struct device *dev)
>  {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 1f3e99c2ef03..27990110fb24 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>   *
>   * This helper implements common pattern present in probe functions for error
>   * checking: print message if the error is not -EPROBE_DEFER and propagate it.
> + * In case of -EPROBE_DEFER it sets defer probe reason.
>   * It replaces code sequence:
>   *     if (err != -EPROBE_DEFER)
>   *             dev_err(dev, ...);
> @@ -3091,14 +3092,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>         struct va_format vaf;
>         va_list args;
>
> -       if (err == -EPROBE_DEFER)
> -               return err;
> -
>         va_start(args, fmt);
>         vaf.fmt = fmt;
>         vaf.va = &args;
>
> -       dev_err(dev, "error %d: %pV", err, &vaf);
> +       if (err == -EPROBE_DEFER)
> +               __deferred_probe_set_msg(dev, &vaf);
> +       else
> +               dev_err(dev, "error %d: %pV", err, &vaf);
>
>         va_end(args);
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..6e488146b328 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/slab.h>
>
>  #include "base.h"
>  #include "power/power.h"
> @@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
>         if (!list_empty(&dev->p->deferred_probe)) {
>                 dev_dbg(dev, "Removed from deferred list\n");
>                 list_del_init(&dev->p->deferred_probe);
> +               kfree(dev->p->deferred_probe_msg);
> +               dev->p->deferred_probe_msg = NULL;
>         }
>         mutex_unlock(&deferred_probe_mutex);
>  }
> @@ -202,6 +205,21 @@ void device_unblock_probing(void)
>         driver_deferred_probe_trigger();
>  }
>
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> + */
> +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
> +{
> +       mutex_lock(&deferred_probe_mutex);
> +

> +       if (dev->p->deferred_probe_msg)

Sorry, I have noticed this a bit late, this check is not needed, since
kfree() is NULL-aware.

> +               kfree(dev->p->deferred_probe_msg);
> +       dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV",
> +                                              dev_driver_string(dev), vaf);

...and perhaps slightly better looking (up to you):

const char *drv = dev_driver_string(dev);

...

... = kasprintf(..., drv, ...); // it would be exactly oneline

> +
> +       mutex_unlock(&deferred_probe_mutex);
> +}
> +
>  /*
>   * deferred_devs_show() - Show the devices in the deferred probe pending list.
>   */
> @@ -212,7 +230,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
>         mutex_lock(&deferred_probe_mutex);
>
>         list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
> -               seq_printf(s, "%s\n", dev_name(curr->device));
> +               seq_printf(s, "%s\t%s", dev_name(curr->device),
> +                          curr->device->p->deferred_probe_msg ?: "\n");
>
>         mutex_unlock(&deferred_probe_mutex);
>
> --
> 2.18.0
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 2/3] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is probe_err function introduced recently,
ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
will be attached to deferred device and printed when user read devices_deferred
property.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v4:
  - removed NULL check before kfree,
  - coding style tweaking.
v3:
  - adjusted deferred_devs_show, to accept newline ended messages,
  - changed conditonal check to positive,
  - added R-b by Andy.
v2:
  - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
  - use kasprintf instead of bunch of code,
  - keep consistent format of devices_deferred lines,
  - added R-Bs (again I hope changes above are not against it).
---
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c |  9 +++++----
 drivers/base/dd.c   | 21 ++++++++++++++++++++-
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 7a419a7a6235..effbd5e7f9f1 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -75,6 +75,7 @@ struct device_private {
 	struct klist_node knode_driver;
 	struct klist_node knode_bus;
 	struct list_head deferred_probe;
+	char *deferred_probe_msg;
 	struct device *device;
 };
 #define to_device_private_parent(obj)	\
@@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
 extern void driver_deferred_probe_del(struct device *dev);
+extern void __deferred_probe_set_msg(const struct device *dev,
+				     struct va_format *vaf);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 1f3e99c2ef03..27990110fb24 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  *
  * This helper implements common pattern present in probe functions for error
  * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason.
  * It replaces code sequence:
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
@@ -3091,14 +3092,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
 	struct va_format vaf;
 	va_list args;
 
-	if (err == -EPROBE_DEFER)
-		return err;
-
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	dev_err(dev, "error %d: %pV", err, &vaf);
+	if (err == -EPROBE_DEFER)
+		__deferred_probe_set_msg(dev, &vaf);
+	else
+		dev_err(dev, "error %d: %pV", err, &vaf);
 
 	va_end(args);
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 169412ee4ae8..e7986a307420 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/slab.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
 	if (!list_empty(&dev->p->deferred_probe)) {
 		dev_dbg(dev, "Removed from deferred list\n");
 		list_del_init(&dev->p->deferred_probe);
+		kfree(dev->p->deferred_probe_msg);
+		dev->p->deferred_probe_msg = NULL;
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
@@ -202,6 +205,21 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/*
+ * __deferred_probe_set_msg() - Set defer probe reason message for device
+ */
+void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
+{
+	const char *drv = dev_driver_string(dev);
+
+	mutex_lock(&deferred_probe_mutex);
+
+	kfree(dev->p->deferred_probe_msg);
+	dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
+
+	mutex_unlock(&deferred_probe_mutex);
+}
+
 /*
  * deferred_devs_show() - Show the devices in the deferred probe pending list.
  */
@@ -212,7 +230,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
 	mutex_lock(&deferred_probe_mutex);
 
 	list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
-		seq_printf(s, "%s\n", dev_name(curr->device));
+		seq_printf(s, "%s\t%s", dev_name(curr->device),
+			   curr->device->p->deferred_probe_msg ?: "\n");
 
 	mutex_unlock(&deferred_probe_mutex);
 
-- 
2.18.0

[PATCH 3/3] drivers: use probe_err function in obvious cases

[Javier Martinez Canillas]

Hello Andrzej,

On 10/16/2018 09:22 AM, Andrzej Hajda wrote:
> The patch replaces obviously matching code with probe_err function.
> There are many more places where probe_err could be used.
> The patch shows how the new function should be used, and how it
> improves the code.
> 
> It was generated by cocci script (little bit dirty):
>

This is great! I guess the easier would be to split these by kernel releases,
maybe patch #1 and #2 can land in one kernel release and then each subsystem
can merge the changes for their drivers with the dependency already in place.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

[PATCH 3/3] drivers: use probe_err function in obvious cases

[Andy Shevchenko]

On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> The patch replaces obviously matching code with probe_err function.
> There are many more places where probe_err could be used.
> The patch shows how the new function should be used, and how it
> improves the code.

> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -824,9 +824,7 @@ static int pca953x_probe(struct i2c_client *client,
>         reg = devm_regulator_get(&client->dev, "vcc");
>         if (IS_ERR(reg)) {
>                 ret = PTR_ERR(reg);
> -               if (ret != -EPROBE_DEFER)
> -                       dev_err(&client->dev, "reg get err: %d\n", ret);
> -               return ret;
> +               return probe_err(&client->dev, ret, "reg get err: %d\n", ret);

No need to assign ret above.
As I mentioned, better to print a value for everyone.

>         }

Please, split it at least one patch per subsystem.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] drivers: use probe_err function in obvious cases

[Andrzej Hajda]

On 16.10.2018 15:51, Andy Shevchenko wrote:
> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> The patch replaces obviously matching code with probe_err function.
>> There are many more places where probe_err could be used.
>> The patch shows how the new function should be used, and how it
>> improves the code.
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -824,9 +824,7 @@ static int pca953x_probe(struct i2c_client *client,
>>         reg = devm_regulator_get(&client->dev, "vcc");
>>         if (IS_ERR(reg)) {
>>                 ret = PTR_ERR(reg);
>> -               if (ret != -EPROBE_DEFER)
>> -                       dev_err(&client->dev, "reg get err: %d\n", ret);
>> -               return ret;
>> +               return probe_err(&client->dev, ret, "reg get err: %d\n", ret);
> No need to assign ret above.
> As I mentioned, better to print a value for everyone.

OK.

Thanks for all reviews. I have posted v2 of patches 1 and 2 as replies
in this thread.
As patch 3 requires splitting, adjusting arguments of probe_err (remove
err and newline), and depends on patch 1 I will prepare it and post
later, probably next cycle as suggested Javier.

Regards
Andrzej


>
>>         }
> Please, split it at least one patch per subsystem.
>