Hello Christian,
On 10/10/25 9:13 AM, Christian Thießen wrote:
> Hi Ahmad, Sascha,
>
> have you reached a conclusion which way you want to take here? Either
> way I'd be happy to help, but as the implementation is rather simple,
> I guess you would be faster doing it yourself than explaining it to me
> and doing a review cycle afterwards.
Commit d37cbb49cc ("net: fec_imx: Fix probe failure on i.MX27") was
included in v2025.09.0 and I assumed it had fixed your issue.
Did you miss it or do you still observe breakage?
Cheers,
Ahmad
>
> Christian
>
>
> On Tue, Sep 16, 2025 at 11:32 AM Ahmad Fatoum <[email protected]> wrote:
>>
>> Hi,
>>
>> On 9/9/25 11:34 AM, Sascha Hauer wrote:
>>> 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().
>>
>> For drivers that are already written, it's easier to designate an error
>> code that shouldn't happen than shift around all existing values and
>> hope you don't miss anything.
>>
>>> 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,
>>> ...
>>> };
>>
>> Linux already has device_get_match_data() with the same semantics we
>> have in barebox now, so if we will not run into additional issues when
>> code is ported from Linux.
>>
>>> 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.
>>
>> That would change the semantics of the function different to what's done
>> in Linux and we don't want that.
>>
>>> Unless Ahmad objects I would prefer converting the in tree users not to
>>> use zero.
>>
>> I still think the approach here is easier to reason about for converting
>> drivers, but if you have invested the time to shift around enumerations
>> in all the drivers, we can go with that too.
>>
>>> Ahmad is on holiday this week, so it'll likely take some time, but
>>> anyway, we'll solve this issue before the next release.
>>
>> Yes, priority is getting this fixed in the next release.
>>
>> Thanks,
>> Ahmad
>>
>>>
>>> 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 |
>>
> The information in this e-mail is confidential. The contents may not be
> disclosed or used by anyone other than the addressee. Access to this e-mail
> by anyone else is unauthorised.
> If you are not the intended recipient, please notify Airbus immediately and
> delete this e-mail.
> Airbus cannot accept any responsibility for the accuracy or completeness of
> this e-mail as it has been sent over public networks. If you have any
> concerns over the content of this message or its Accuracy or Integrity,
> please contact Airbus immediately.
> All outgoing e-mails from Airbus are checked using regularly updated virus
> scanning software but you should take whatever measures you deem to be
> appropriate to ensure that this message and any attachments are virus free.
--
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 |
