Re: [PATCH] fec_imx: Fix ENODEV in fec_probe on i.MX27

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Christian,

On Wed, Sep 03, 2025 at 04:46:05PM +0200, Christian Thießen via B4 Relay wrote:
> From: Christian Thießen <christian.thiessen@airbus.com>
> 
> The fec_imx driver stores an enum fec_type value in the match data
> pointer. When retrieving this value in fec_probe(), it handles
> FEC_TYPE_IMX27 (which is 0) like an error result from
> device_get_match_data and returns -ENODEV.
> 
> To be able to differentiate NULL from errors, introduce a new function
> device_get_match_data_default which returns its defaultval argument
> instead of NULL in case of errors.
> Add an FEC_TYPE_COUNT value to the fec_type enum.
> In fec_probe, pass FEC_TYPE_COUNT as defaultval and return -ENODEV for
> all type values greater or equal to that.
> 
> Fixes: 20d87123a6 ("treewide: replace dev_get_drvdata with        device_get_match_data")
> Signed-off-by: Christian Thießen <christian.thiessen@airbus.com>
> ---
> Hi again,
> 
> this patch addresses the issue discussed in
> https://github.com/barebox/barebox/issues/40 ("fec_imx (and possibly
> others): Probe fails with ENODEV").
> 
> Cheers,
> Christian
> ---
>  drivers/base/driver.c | 13 +++++++++++++
>  drivers/net/fec_imx.c | 12 +++++-------
>  drivers/net/fec_imx.h |  1 +
>  include/driver.h      | 10 ++++++++++
>  4 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 61118385f1df0f8d36208d236be66b077fa5e54d..614c3c26b04c6f252e3970e014a43bae6a1d9c2b 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -690,6 +690,19 @@ const void *device_get_match_data(struct device *dev)
>  }
>  EXPORT_SYMBOL(device_get_match_data);
>  
> +const void *device_get_match_data_default(struct device *dev,
> +					  const void *defaultval)
> +{
> +	if (dev->of_id_entry)
> +		return dev->of_id_entry->data;
> +
> +	if (dev->id_entry)
> +		return (const void *)dev->id_entry->driver_data;
> +
> +	return defaultval;
> +}
> +EXPORT_SYMBOL(device_get_match_data_default);

This more goes to Ahmad, because he suggested this solution:

I don't like this very much. With this solution the driver writer must
still know that he has to call device_get_match_data_default() instead
of plain device_get_match_data().

In other words, the driver writer must be aware of this problem, so he
could equally well avoid 0 as valid value and declare

enum fec_type {
	FEC_TYPE_IMX27 = 1,
	...
};

in this case. I have looked up all cases that pass an integer into data
in the tree and these are not many, we could change them easily.

Another possibility would be to return an error pointer instead of NULL
from device_get_match_data(), but that would require some refactoring
throughout the tree.

Unless Ahmad objects I would prefer converting the in tree users not to
use zero.

Ahmad is on holiday this week, so it'll likely take some time, but
anyway, we'll solve this issue before the next release.

Sascha


> +
>  static void device_set_deferred_probe_reason(struct device *dev,
>  					     const struct va_format *vaf)
>  {
> diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
> index 8474e6d5be9c6225af5708d21b0249b2618d36f6..fac190e6ea51950ed6085264e12617969847acbf 100644
> --- a/drivers/net/fec_imx.c
> +++ b/drivers/net/fec_imx.c
> @@ -763,20 +763,18 @@ static int fec_probe(struct device *dev)
>  	struct fec_priv *fec;
>  	void *base;
>  	int ret;
> -	enum fec_type type;
> -	void const *type_v;
> +	uintptr_t type_v;
>  	int phy_reset;
>  	u32 msec = 1, phy_post_delay = 0;
>  	u32 reg;
>  
> -	type_v = device_get_match_data(dev);
> -	if (!type_v)
> +	type_v = (uintptr_t)device_get_match_data_default(dev,
> +		(const void *)FEC_TYPE_COUNT);
> +	if (type_v >= FEC_TYPE_COUNT)
>  		return -ENODEV;
>  
> -	type = (uintptr_t)(type_v);
> -
>  	fec = xzalloc(sizeof(*fec));
> -	fec->type = type;
> +	fec->type = type_v;
>  	fec->dev = dev;
>  	edev = &fec->edev;
>  	dev->priv = fec;
> diff --git a/drivers/net/fec_imx.h b/drivers/net/fec_imx.h
> index 1aaff87fddfb44cf558436ab4a846757c6cd51aa..9549dab0f8e88288bd9fe3a02ff8bd5573147842 100644
> --- a/drivers/net/fec_imx.h
> +++ b/drivers/net/fec_imx.h
> @@ -117,6 +117,7 @@ enum fec_type {
>  	FEC_TYPE_IMX27,
>  	FEC_TYPE_IMX28,
>  	FEC_TYPE_IMX6,
> +	FEC_TYPE_COUNT,
>  };
>  
>  enum fec_clock {
> diff --git a/include/driver.h b/include/driver.h
> index c130a3cd63fd5e6dc365a33c765a22dc4e2ca90a..223799294fc0e32ceb13fc5bda2e3b8a78d71647 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -650,6 +650,16 @@ int cdevfs_del_partition(struct cdev *cdev);
>   */
>  const void *device_get_match_data(struct device *dev);
>  
> +/**
> + * device_get_match_data_default - get driver match data associated with device
> + * @dev: device instance
> + * @defaultval: Value to return if no match data is associated with dev
> + *
> + * Returns match data on success and defaultval otherwise
> + */
> +const void *device_get_match_data_default(struct device *dev,
> +					  const void *defaultval);
> +
>  int device_match_of_modalias(struct device *dev, const struct driver *drv);
>  
>  struct device *device_find_child(struct device *parent, void *data,
> 
> ---
> base-commit: 7689314d0a78605e7d4a0c3ce4953d9d768aa343
> change-id: 20250903-fec_imx-fix-enodev-f7a9087054c8
> 
> Best regards,
> -- 
> Christian Thießen <christian.thiessen@airbus.com>
> 
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |