Hi Christian,
On Wed, Sep 03, 2025 at 04:46:05PM +0200, Christian Thießen via B4 Relay wrote:
> From: Christian Thießen <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>
>
>
>
--
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 |
