Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On Fri, Jul 10, 2020 at 09:42:49AM +0200, Andrzej Hajda wrote: > But the provider does not know if *get is called in probe context or > not, so it is not able to differentiate it. > So the whole idea is for me suspicious/wrong. Kind of proof: > 1. If you insist that provider's EPROBE_ERROR must be always propagated > to driver core then. > 2. You must enforce that resources can be gathered only from probe. > 3. But this is against current practice, even if majority of drivers > does it from probe, there are many which doesn't. Those drivers are probably buggy anyway at this point given probe deferral. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On 07.07.2020 06:14, Dmitry Torokhov wrote:
> On Thu, Jul 02, 2020 at 08:57:55AM +0200, Andrzej Hajda wrote:
>> On 30.06.2020 20:00, Dmitry Torokhov wrote:
>>> On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda wrote:
On 30.06.2020 10:59, Grygorii Strashko wrote:
> Hi
>
> On 29/06/2020 14:28, Andrzej Hajda wrote:
>> Hi Grygorii,
>>
>> (...)
>>
/*
* deferred_devs_show() - Show the devices in the deferred probe
pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
void *data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _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_reason ?: "\n");
mutex_unlock(_probe_mutex);
>>> Sry, may be i missing smth, but shouldn't it be optional
>>> (CONFIG_DEBUG_FS is probably too generic).
>>>
>> I am not sure what exactly are you referring to, but this patch does not
>> add new property, it just extends functionality of existing one.
> Sry, needed to be more specific.
>
> You've added device_set_deferred_probe_reson(dev, );
> which expected to be used on every EPROBE_DEFER in dev_err_probe() in
> combination with
>
> + } else {
> + device_set_deferred_probe_reson(dev, );
> dev_dbg(dev, "error %d: %pV", err, );
>
> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
> + }
>
> But:
>
> +void device_set_deferred_probe_reson(const struct device *dev, struct
> va_format *vaf)
> +{
> + const char *drv = dev_driver_string(dev);
> +
> + mutex_lock(_probe_mutex);
> +
> + kfree(dev->p->deferred_probe_reason);
> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
> %pV", drv, vaf);
> +
> + mutex_unlock(_probe_mutex);
> +}
>
> ^^ Adds locking, kfree() and kasprintf() for every deferred probe
> during boot and can't be disabled.
>
> Right?
Right, but usually the burden should be insignificant in comparison to
probe time, so I do not think it is worth optimizing.
>>> I do not think this is going to take. You are suggesting that we
>>> modify pretty much every driver to supply this deferral reason, and I
>>> doubt it will happen. Can we put this burden on providers that raise
>>> the deferral?
>>
>> I wouldn't say they raise the deferral, they just inform resource is not
>> yet available. Only device driver, and only in its probe function can
>> "raise the deferral".
> Well, this is a matter of perspective. If devm_gpiod_get() returns
> -EBUSY and this is returned to driver core, is it GPIO line signals that
> line is busy, or is it the driver applies its knowledge. I say that in
> majority of cases driver does not really get a say in this and simply
> has to pass whatever error condition that is signalled by providers up
> the stack.
>
> I would consider whenever a driver does not propagate -EPROBE_DEFER to
> the driver code a bug that needs fixing, because it should not degrade
> functionality and/or performance just because we have not figured out
> how to order probing properly and have to rely on deferrals.
>
>>
>>>I.e. majority of code are using devm API now, so we most
>>> likely know the device for which deferral is being raised. We can have
>>> a list of deferral reasons and their devices and when in device code
>>> once probe is done we could try reconciling it with the deferred
>>> devicelist, and this would mean you only need to implement this in
>>> gpiolib, regulator core, clocks core, etc.
>>
>> This patchset tries to solve simple issue - replace multiple lines of
>> code present in multiple probe functions (additionally fixing lot of
>> them) with single call and then enhance it little bit, nothing more.
>>
>> What you are proposing is blurry at the moment for me, provider does not
>> know if consumer want to defer,
> This is my point - the consumer does not get to decide. If deferral is
> raised, it must be honored.
>
>> or will continue working without missing resource,
> Deferral does not mean resource does not exist and the driver has to get
> by if it can. It means the resource is not ready, and even if the system
> can work without it, it will not be working optimally.
>
>> moreover some consumers can acquire resources after probe - again no
>> probe deferral.
> In this case we should not signal deferral either.
But the provider does not know if *get is called in probe context or
not, so it is not able to differentiate it.
So the whole idea is for me suspicious/wrong. Kind of proof:
1. If you insist that provider's EPROBE_ERROR
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On Thu, Jul 02, 2020 at 08:57:55AM +0200, Andrzej Hajda wrote:
>
> On 30.06.2020 20:00, Dmitry Torokhov wrote:
> > On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda wrote:
> >>
> >> On 30.06.2020 10:59, Grygorii Strashko wrote:
> >>> Hi
> >>>
> >>> On 29/06/2020 14:28, Andrzej Hajda wrote:
> Hi Grygorii,
>
> (...)
>
> >> /*
> >> * deferred_devs_show() - Show the devices in the deferred probe
> >> pending list.
> >> */
> >> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
> >> void *data)
> >> mutex_lock(_probe_mutex);
> >> list_for_each_entry(curr, _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_reason ?: "\n");
> >> mutex_unlock(_probe_mutex);
> >>
> > Sry, may be i missing smth, but shouldn't it be optional
> > (CONFIG_DEBUG_FS is probably too generic).
> >
> I am not sure what exactly are you referring to, but this patch does not
> add new property, it just extends functionality of existing one.
> >>> Sry, needed to be more specific.
> >>>
> >>> You've added device_set_deferred_probe_reson(dev, );
> >>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in
> >>> combination with
> >>>
> >>> + } else {
> >>> + device_set_deferred_probe_reson(dev, );
> >>> dev_dbg(dev, "error %d: %pV", err, );
> >>>
> >>> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
> >>> + }
> >>>
> >>> But:
> >>>
> >>> +void device_set_deferred_probe_reson(const struct device *dev, struct
> >>> va_format *vaf)
> >>> +{
> >>> + const char *drv = dev_driver_string(dev);
> >>> +
> >>> + mutex_lock(_probe_mutex);
> >>> +
> >>> + kfree(dev->p->deferred_probe_reason);
> >>> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
> >>> %pV", drv, vaf);
> >>> +
> >>> + mutex_unlock(_probe_mutex);
> >>> +}
> >>>
> >>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe
> >>> during boot and can't be disabled.
> >>>
> >>> Right?
> >>
> >> Right, but usually the burden should be insignificant in comparison to
> >> probe time, so I do not think it is worth optimizing.
> > I do not think this is going to take. You are suggesting that we
> > modify pretty much every driver to supply this deferral reason, and I
> > doubt it will happen. Can we put this burden on providers that raise
> > the deferral?
>
>
> I wouldn't say they raise the deferral, they just inform resource is not
> yet available. Only device driver, and only in its probe function can
> "raise the deferral".
Well, this is a matter of perspective. If devm_gpiod_get() returns
-EBUSY and this is returned to driver core, is it GPIO line signals that
line is busy, or is it the driver applies its knowledge. I say that in
majority of cases driver does not really get a say in this and simply
has to pass whatever error condition that is signalled by providers up
the stack.
I would consider whenever a driver does not propagate -EPROBE_DEFER to
the driver code a bug that needs fixing, because it should not degrade
functionality and/or performance just because we have not figured out
how to order probing properly and have to rely on deferrals.
>
>
> > I.e. majority of code are using devm API now, so we most
> > likely know the device for which deferral is being raised. We can have
> > a list of deferral reasons and their devices and when in device code
> > once probe is done we could try reconciling it with the deferred
> > devicelist, and this would mean you only need to implement this in
> > gpiolib, regulator core, clocks core, etc.
>
>
> This patchset tries to solve simple issue - replace multiple lines of
> code present in multiple probe functions (additionally fixing lot of
> them) with single call and then enhance it little bit, nothing more.
>
> What you are proposing is blurry at the moment for me, provider does not
> know if consumer want to defer,
This is my point - the consumer does not get to decide. If deferral is
raised, it must be honored.
> or will continue working without missing resource,
Deferral does not mean resource does not exist and the driver has to get
by if it can. It means the resource is not ready, and even if the system
can work without it, it will not be working optimally.
> moreover some consumers can acquire resources after probe - again no
> probe deferral.
In this case we should not signal deferral either.
> Even if it will be done (it can be, for
> example by creating probe version of all resource get functions), it
> will require much more changes but finally it will look like:
>
> res = devm_get_resource_from_probe()
>
> if (IS_ERR(res))
>
> return PTR_ERR(res);
>
> vs:
>
> res
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On 30.06.2020 20:00, Dmitry Torokhov wrote:
> On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda wrote:
>>
>> On 30.06.2020 10:59, Grygorii Strashko wrote:
>>> Hi
>>>
>>> On 29/06/2020 14:28, Andrzej Hajda wrote:
Hi Grygorii,
(...)
>> /*
>> * deferred_devs_show() - Show the devices in the deferred probe
>> pending list.
>> */
>> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
>> void *data)
>> mutex_lock(_probe_mutex);
>> list_for_each_entry(curr, _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_reason ?: "\n");
>> mutex_unlock(_probe_mutex);
>>
> Sry, may be i missing smth, but shouldn't it be optional
> (CONFIG_DEBUG_FS is probably too generic).
>
I am not sure what exactly are you referring to, but this patch does not
add new property, it just extends functionality of existing one.
>>> Sry, needed to be more specific.
>>>
>>> You've added device_set_deferred_probe_reson(dev, );
>>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in
>>> combination with
>>>
>>> + } else {
>>> + device_set_deferred_probe_reson(dev, );
>>> dev_dbg(dev, "error %d: %pV", err, );
>>>
>>> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
>>> + }
>>>
>>> But:
>>>
>>> +void device_set_deferred_probe_reson(const struct device *dev, struct
>>> va_format *vaf)
>>> +{
>>> + const char *drv = dev_driver_string(dev);
>>> +
>>> + mutex_lock(_probe_mutex);
>>> +
>>> + kfree(dev->p->deferred_probe_reason);
>>> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
>>> %pV", drv, vaf);
>>> +
>>> + mutex_unlock(_probe_mutex);
>>> +}
>>>
>>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe
>>> during boot and can't be disabled.
>>>
>>> Right?
>>
>> Right, but usually the burden should be insignificant in comparison to
>> probe time, so I do not think it is worth optimizing.
> I do not think this is going to take. You are suggesting that we
> modify pretty much every driver to supply this deferral reason, and I
> doubt it will happen. Can we put this burden on providers that raise
> the deferral?
I wouldn't say they raise the deferral, they just inform resource is not
yet available. Only device driver, and only in its probe function can
"raise the deferral".
> I.e. majority of code are using devm API now, so we most
> likely know the device for which deferral is being raised. We can have
> a list of deferral reasons and their devices and when in device code
> once probe is done we could try reconciling it with the deferred
> devicelist, and this would mean you only need to implement this in
> gpiolib, regulator core, clocks core, etc.
This patchset tries to solve simple issue - replace multiple lines of
code present in multiple probe functions (additionally fixing lot of
them) with single call and then enhance it little bit, nothing more.
What you are proposing is blurry at the moment for me, provider does not
know if consumer want to defer, or will continue working without
missing resource, moreover some consumers can acquire resources after
probe - again no probe deferral. Even if it will be done (it can be, for
example by creating probe version of all resource get functions), it
will require much more changes but finally it will look like:
res = devm_get_resource_from_probe()
if (IS_ERR(res))
return PTR_ERR(res);
vs:
res = devm_get_resource(...)
if (IS_ERR(res))
return dev_err_probe(dev, PTR_ERR(res), ...);
Regards
Andrzej
>
> Thanks.
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
Hi
On 29/06/2020 14:28, Andrzej Hajda wrote:
Hi Grygorii,
(...)
/*
* deferred_devs_show() - Show the devices in the deferred probe
pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
void *data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _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_reason ?: "\n");
mutex_unlock(_probe_mutex);
Sry, may be i missing smth, but shouldn't it be optional
(CONFIG_DEBUG_FS is probably too generic).
I am not sure what exactly are you referring to, but this patch does not
add new property, it just extends functionality of existing one.
Sry, needed to be more specific.
You've added device_set_deferred_probe_reson(dev, );
which expected to be used on every EPROBE_DEFER in dev_err_probe() in
combination with
+ } else {
+ device_set_deferred_probe_reson(dev, );
dev_dbg(dev, "error %d: %pV", err, );
^^ dev_dbg() does not add any runtime overhead during boot unless enabled
+ }
But:
+void device_set_deferred_probe_reson(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
^^ Adds locking, kfree() and kasprintf() for every deferred probe during boot
and can't be disabled.
Right?
--
Best regards,
grygorii
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda wrote:
>
>
> On 30.06.2020 10:59, Grygorii Strashko wrote:
> > Hi
> >
> > On 29/06/2020 14:28, Andrzej Hajda wrote:
> >> Hi Grygorii,
> >>
> >> (...)
> >>
> /*
> * deferred_devs_show() - Show the devices in the deferred probe
> pending list.
> */
> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
> void *data)
> mutex_lock(_probe_mutex);
> list_for_each_entry(curr, _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_reason ?: "\n");
> mutex_unlock(_probe_mutex);
>
> >>>
> >>> Sry, may be i missing smth, but shouldn't it be optional
> >>> (CONFIG_DEBUG_FS is probably too generic).
> >>>
> >>
> >> I am not sure what exactly are you referring to, but this patch does not
> >> add new property, it just extends functionality of existing one.
> >
> > Sry, needed to be more specific.
> >
> > You've added device_set_deferred_probe_reson(dev, );
> > which expected to be used on every EPROBE_DEFER in dev_err_probe() in
> > combination with
> >
> > + } else {
> > + device_set_deferred_probe_reson(dev, );
> > dev_dbg(dev, "error %d: %pV", err, );
> >
> > ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
> > + }
> >
> > But:
> >
> > +void device_set_deferred_probe_reson(const struct device *dev, struct
> > va_format *vaf)
> > +{
> > + const char *drv = dev_driver_string(dev);
> > +
> > + mutex_lock(_probe_mutex);
> > +
> > + kfree(dev->p->deferred_probe_reason);
> > + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
> > %pV", drv, vaf);
> > +
> > + mutex_unlock(_probe_mutex);
> > +}
> >
> > ^^ Adds locking, kfree() and kasprintf() for every deferred probe
> > during boot and can't be disabled.
> >
> > Right?
>
>
> Right, but usually the burden should be insignificant in comparison to
> probe time, so I do not think it is worth optimizing.
I do not think this is going to take. You are suggesting that we
modify pretty much every driver to supply this deferral reason, and I
doubt it will happen. Can we put this burden on providers that raise
the deferral? I.e. majority of code are using devm API now, so we most
likely know the device for which deferral is being raised. We can have
a list of deferral reasons and their devices and when in device code
once probe is done we could try reconciling it with the deferred
devicelist, and this would mean you only need to implement this in
gpiolib, regulator core, clocks core, etc.
Thanks.
--
Dmitry
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On 30.06.2020 10:59, Grygorii Strashko wrote:
> Hi
>
> On 29/06/2020 14:28, Andrzej Hajda wrote:
>> Hi Grygorii,
>>
>> (...)
>>
/*
* deferred_devs_show() - Show the devices in the deferred probe
pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
void *data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _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_reason ?: "\n");
mutex_unlock(_probe_mutex);
>>>
>>> Sry, may be i missing smth, but shouldn't it be optional
>>> (CONFIG_DEBUG_FS is probably too generic).
>>>
>>
>> I am not sure what exactly are you referring to, but this patch does not
>> add new property, it just extends functionality of existing one.
>
> Sry, needed to be more specific.
>
> You've added device_set_deferred_probe_reson(dev, );
> which expected to be used on every EPROBE_DEFER in dev_err_probe() in
> combination with
>
> + } else {
> + device_set_deferred_probe_reson(dev, );
> dev_dbg(dev, "error %d: %pV", err, );
>
> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
> + }
>
> But:
>
> +void device_set_deferred_probe_reson(const struct device *dev, struct
> va_format *vaf)
> +{
> + const char *drv = dev_driver_string(dev);
> +
> + mutex_lock(_probe_mutex);
> +
> + kfree(dev->p->deferred_probe_reason);
> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
> %pV", drv, vaf);
> +
> + mutex_unlock(_probe_mutex);
> +}
>
> ^^ Adds locking, kfree() and kasprintf() for every deferred probe
> during boot and can't be disabled.
>
> Right?
Right, but usually the burden should be insignificant in comparison to
probe time, so I do not think it is worth optimizing.
Regards
Andrzej
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
Hi Grygorii, (...) >> /* >> * deferred_devs_show() - Show the devices in the deferred probe >> pending list. >> */ >> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, >> void *data) >> mutex_lock(_probe_mutex); >> list_for_each_entry(curr, _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_reason ?: "\n"); >> mutex_unlock(_probe_mutex); >> > > Sry, may be i missing smth, but shouldn't it be optional > (CONFIG_DEBUG_FS is probably too generic). > I am not sure what exactly are you referring to, but this patch does not add new property, it just extends functionality of existing one. Regards Andrzej ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On 26/06/2020 13:01, 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
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 8 ++--
drivers/base/dd.c | 23 ++-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_reason;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,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 device_set_deferred_probe_reson(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 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
* This helper implements common pattern present in probe functions for error
* checking: print debug or error message depending if the error value is
* -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
*if (err != -EPROBE_DEFER)
*dev_err(dev, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
const char *fmt, ...)
vaf.fmt = fmt;
vaf.va =
- if (err != -EPROBE_DEFER)
+ if (err != -EPROBE_DEFER) {
dev_err(dev, "error %d: %pV", err, );
- else
+ } else {
+ device_set_deferred_probe_reson(dev, );
dev_dbg(dev, "error %d: %pV", err, );
+ }
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for
device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _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_reason ?: "\n");
mutex_unlock(_probe_mutex);
Sry, may be i missing smth, but shouldn't it be optional
(CONFIG_DEBUG_FS is probably too generic).
--
Best regards,
grygorii
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On Fri, Jun 26, 2020 at 12:01 PM 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
> Reviewed-by: Mark Brown
> Reviewed-by: Javier Martinez Canillas
> Reviewed-by: Andy Shevchenko
Reviewed-by: Rafael J. Wysocki
> ---
> drivers/base/base.h | 3 +++
> drivers/base/core.c | 8 ++--
> drivers/base/dd.c | 23 ++-
> 3 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 95c22c0f9036..6954fccab3d7 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -93,6 +93,7 @@ struct device_private {
> struct klist_node knode_class;
> struct list_head deferred_probe;
> struct device_driver *async_driver;
> + char *deferred_probe_reason;
> struct device *device;
> u8 dead:1;
> };
> @@ -134,6 +135,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 device_set_deferred_probe_reson(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 3a827c82933f..fee047f03681 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
> * This helper implements common pattern present in probe functions for error
> * checking: print debug or error message depending if the error value is
> * -EPROBE_DEFER and propagate error upwards.
> + * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
> + * checked later by reading devices_deferred debugfs attribute.
> * It replaces code sequence:
> * if (err != -EPROBE_DEFER)
> * dev_err(dev, ...);
> @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
> const char *fmt, ...)
> vaf.fmt = fmt;
> vaf.va =
>
> - if (err != -EPROBE_DEFER)
> + if (err != -EPROBE_DEFER) {
> dev_err(dev, "error %d: %pV", err, );
> - else
> + } else {
> + device_set_deferred_probe_reson(dev, );
> dev_dbg(dev, "error %d: %pV", err, );
> + }
>
> va_end(args);
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9a1d940342ac..dd5683b61f74 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include "base.h"
> #include "power/power.h"
> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
> if (!list_empty(>p->deferred_probe)) {
> dev_dbg(dev, "Removed from deferred list\n");
> list_del_init(>p->deferred_probe);
> + kfree(dev->p->deferred_probe_reason);
> + dev->p->deferred_probe_reason = NULL;
> }
> mutex_unlock(_probe_mutex);
> }
> @@ -211,6 +214,23 @@ void device_unblock_probing(void)
> driver_deferred_probe_trigger();
> }
>
> +/**
> + * device_set_deferred_probe_reson() - Set defer probe reason message for
> device
> + * @dev: the pointer to the struct device
> + * @vaf: the pointer to va_format structure with message
> + */
> +void device_set_deferred_probe_reson(const struct device *dev, struct
> va_format *vaf)
> +{
> + const char *drv = dev_driver_string(dev);
> +
> + mutex_lock(_probe_mutex);
> +
> + kfree(dev->p->deferred_probe_reason);
> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
> vaf);
> +
> + mutex_unlock(_probe_mutex);
> +}
> +
> /*
> * deferred_devs_show() - Show the devices in the deferred probe pending
> list.
> */
> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
> *data)
> mutex_lock(_probe_mutex);
>
> list_for_each_entry(curr, _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_reason ?: "\n");
>
> mutex_unlock(_probe_mutex);
>
> --
> 2.17.1
>
___
dri-devel mailing list
[PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
/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
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 8 ++--
drivers/base/dd.c | 23 ++-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_reason;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,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 device_set_deferred_probe_reson(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 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
* This helper implements common pattern present in probe functions for error
* checking: print debug or error message depending if the error value is
* -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
* if (err != -EPROBE_DEFER)
* dev_err(dev, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
const char *fmt, ...)
vaf.fmt = fmt;
vaf.va =
- if (err != -EPROBE_DEFER)
+ if (err != -EPROBE_DEFER) {
dev_err(dev, "error %d: %pV", err, );
- else
+ } else {
+ device_set_deferred_probe_reson(dev, );
dev_dbg(dev, "error %d: %pV", err, );
+ }
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for
device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _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_reason ?: "\n");
mutex_unlock(_probe_mutex);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
