On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
> only; that status being determined through the .device_is_available() op
> of the device's fwnode. As software_nodes don't have that operation and
> adding it is meaningless, we instead need to check if the device's fwnode
> is a software_node and if so pass the appropriate flag to disable that
> check
Period.
I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
> > fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
> > only; that status being determined through the .device_is_available() op
> > of the device's fwnode. As software_nodes don't have that operation and
> > adding it is meaningless, we instead need to check if the device's fwnode
> > is a software_node and if so pass the appropriate flag to disable that
> > check
>
> Period.
>
> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
The device availability test is actually there for a reason. Some firmware
implementations put all the potential devices in the tables and only one
(of some) of them are available.
Could this be implemented so that if the node is a software node, then get
its parent and then see if that is available?
I guess that could be implemented in software node ops. Any opinions?
--
Kind regards,
Sakari Ailus
Hi Sakari
On 20/10/2020 13:06, Sakari Ailus wrote:
> Hi Andy,
>
> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
>>> only; that status being determined through the .device_is_available() op
>>> of the device's fwnode. As software_nodes don't have that operation and
>>> adding it is meaningless, we instead need to check if the device's fwnode
>>> is a software_node and if so pass the appropriate flag to disable that
>>> check
>> Period.
>>
>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
> The device availability test is actually there for a reason. Some firmware
> implementations put all the potential devices in the tables and only one
> (of some) of them are available.
>
> Could this be implemented so that if the node is a software node, then get
> its parent and then see if that is available?
>
> I guess that could be implemented in software node ops. Any opinions?
Actually when considering the cio2 device, it seems that
set_secondary_fwnode() actually overwrites the _primary_, given
fwnode_is_primary(dev->fwnode) returns false. So in at least some cases,
this wouldn't work.
Hi Dan,
On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote:
> Hi Sakari
>
> On 20/10/2020 13:06, Sakari Ailus wrote:
> > Hi Andy,
> >
> > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
> >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
> >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
> >>> only; that status being determined through the .device_is_available() op
> >>> of the device's fwnode. As software_nodes don't have that operation and
> >>> adding it is meaningless, we instead need to check if the device's fwnode
> >>> is a software_node and if so pass the appropriate flag to disable that
> >>> check
> >> Period.
> >>
> >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
> > The device availability test is actually there for a reason. Some firmware
> > implementations put all the potential devices in the tables and only one
> > (of some) of them are available.
> >
> > Could this be implemented so that if the node is a software node, then get
> > its parent and then see if that is available?
> >
> > I guess that could be implemented in software node ops. Any opinions?
> Actually when considering the cio2 device, it seems that
> set_secondary_fwnode() actually overwrites the _primary_, given
> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases,
> this wouldn't work.
Ouch. I wonder when this happens --- have you checked what's the primary
there? I guess it might be if it's a PCI device without the corresponding
ACPI device node?
I remember you had an is_available implementation that just returned true
for software nodes in an early version of the set? I think it would still
be a lesser bad in this case.
--
Regards,
Sakari Ailus
Hi Sakari
On 20/10/2020 23:49, Sakari Ailus wrote:
> Hi Dan,
>
> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote:
>> Hi Sakari
>>
>> On 20/10/2020 13:06, Sakari Ailus wrote:
>>> Hi Andy,
>>>
>>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
>>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
>>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
>>>>> only; that status being determined through the .device_is_available() op
>>>>> of the device's fwnode. As software_nodes don't have that operation and
>>>>> adding it is meaningless, we instead need to check if the device's fwnode
>>>>> is a software_node and if so pass the appropriate flag to disable that
>>>>> check
>>>> Period.
>>>>
>>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
>>> The device availability test is actually there for a reason. Some firmware
>>> implementations put all the potential devices in the tables and only one
>>> (of some) of them are available.
>>>
>>> Could this be implemented so that if the node is a software node, then get
>>> its parent and then see if that is available?
>>>
>>> I guess that could be implemented in software node ops. Any opinions?
>> Actually when considering the cio2 device, it seems that
>> set_secondary_fwnode() actually overwrites the _primary_, given
>> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases,
>> this wouldn't work.
> Ouch. I wonder when this happens --- have you checked what's the primary
> there? I guess it might be if it's a PCI device without the corresponding
> ACPI device node?
Yes; it's null, and I think that diagnosis is correct.
> I remember you had an is_available implementation that just returned true
> for software nodes in an early version of the set? I think it would still
> be a lesser bad in this case.
Yep - I can put that back in and just drop this patch then; fine for me.
Hi Sakari
On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote:
> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote:
> > On 20/10/2020 13:06, Sakari Ailus wrote:
> > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
> > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
> > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
> > >>> only; that status being determined through the .device_is_available() op
> > >>> of the device's fwnode. As software_nodes don't have that operation and
> > >>> adding it is meaningless, we instead need to check if the device's fwnode
> > >>> is a software_node and if so pass the appropriate flag to disable that
> > >>> check
> > >> Period.
> > >>
> > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
> > > The device availability test is actually there for a reason. Some firmware
> > > implementations put all the potential devices in the tables and only one
> > > (of some) of them are available.
> > >
> > > Could this be implemented so that if the node is a software node, then get
> > > its parent and then see if that is available?
> > >
> > > I guess that could be implemented in software node ops. Any opinions?
> > Actually when considering the cio2 device, it seems that
> > set_secondary_fwnode() actually overwrites the _primary_, given
> > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases,
> > this wouldn't work.
>
> Ouch. I wonder when this happens --- have you checked what's the primary
> there? I guess it might be if it's a PCI device without the corresponding
> ACPI device node?
>
> I remember you had an is_available implementation that just returned true
> for software nodes in an early version of the set? I think it would still
> be a lesser bad in this case.
How about the following ?
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 81bd01ed4042..ea44ba846299 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
/**
* fwnode_device_is_available - check if a device is available for use
* @fwnode: Pointer to the fwnode of the device.
+ *
+ * For fwnode node types that don't implement the .device_is_available()
+ * operation, such as software nodes, this function returns true.
*/
bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
{
+ if (!fwnode_has_op(fwnode, device_is_available))
+ return true;
return fwnode_call_bool_op(fwnode, device_is_available);
}
EXPORT_SYMBOL_GPL(fwnode_device_is_available);
--
Regards,
Laurent Pinchart
On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote:
> Hi Sakari
>
> On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote:
> > On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote:
> > > On 20/10/2020 13:06, Sakari Ailus wrote:
> > > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
> > > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
> > > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
> > > >>> only; that status being determined through the .device_is_available() op
> > > >>> of the device's fwnode. As software_nodes don't have that operation and
> > > >>> adding it is meaningless, we instead need to check if the device's fwnode
> > > >>> is a software_node and if so pass the appropriate flag to disable that
> > > >>> check
> > > >> Period.
> > > >>
> > > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
> > > > The device availability test is actually there for a reason. Some firmware
> > > > implementations put all the potential devices in the tables and only one
> > > > (of some) of them are available.
> > > >
> > > > Could this be implemented so that if the node is a software node, then get
> > > > its parent and then see if that is available?
> > > >
> > > > I guess that could be implemented in software node ops. Any opinions?
> > > Actually when considering the cio2 device, it seems that
> > > set_secondary_fwnode() actually overwrites the _primary_, given
> > > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases,
> > > this wouldn't work.
> >
> > Ouch. I wonder when this happens --- have you checked what's the primary
> > there? I guess it might be if it's a PCI device without the corresponding
> > ACPI device node?
> >
> > I remember you had an is_available implementation that just returned true
> > for software nodes in an early version of the set? I think it would still
> > be a lesser bad in this case.
>
> How about the following ?
Looks good to me.
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 81bd01ed4042..ea44ba846299 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
> /**
> * fwnode_device_is_available - check if a device is available for use
> * @fwnode: Pointer to the fwnode of the device.
> + *
> + * For fwnode node types that don't implement the .device_is_available()
> + * operation, such as software nodes, this function returns true.
> */
> bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
> {
> + if (!fwnode_has_op(fwnode, device_is_available))
> + return true;
> return fwnode_call_bool_op(fwnode, device_is_available);
> }
> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
>
--
Sakari Ailus
On 24/10/2020 15:29, Sakari Ailus wrote:
> On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote:
>> Hi Sakari
>>
>> On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote:
>>> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote:
>>>> On 20/10/2020 13:06, Sakari Ailus wrote:
>>>>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
>>>>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
>>>>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
>>>>>>> only; that status being determined through the .device_is_available() op
>>>>>>> of the device's fwnode. As software_nodes don't have that operation and
>>>>>>> adding it is meaningless, we instead need to check if the device's fwnode
>>>>>>> is a software_node and if so pass the appropriate flag to disable that
>>>>>>> check
>>>>>> Period.
>>>>>>
>>>>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
>>>>> The device availability test is actually there for a reason. Some firmware
>>>>> implementations put all the potential devices in the tables and only one
>>>>> (of some) of them are available.
>>>>>
>>>>> Could this be implemented so that if the node is a software node, then get
>>>>> its parent and then see if that is available?
>>>>>
>>>>> I guess that could be implemented in software node ops. Any opinions?
>>>> Actually when considering the cio2 device, it seems that
>>>> set_secondary_fwnode() actually overwrites the _primary_, given
>>>> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases,
>>>> this wouldn't work.
>>> Ouch. I wonder when this happens --- have you checked what's the primary
>>> there? I guess it might be if it's a PCI device without the corresponding
>>> ACPI device node?
>>>
>>> I remember you had an is_available implementation that just returned true
>>> for software nodes in an early version of the set? I think it would still
>>> be a lesser bad in this case.
>> How about the following ?
> Looks good to me.
If we're agreed on this (and it's fine by me too), do you want me to
include it in the next set, or are you going to do it separately Laurent?
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 81bd01ed4042..ea44ba846299 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
>> /**
>> * fwnode_device_is_available - check if a device is available for use
>> * @fwnode: Pointer to the fwnode of the device.
>> + *
>> + * For fwnode node types that don't implement the .device_is_available()
>> + * operation, such as software nodes, this function returns true.
>> */
>> bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
>> {
>> + if (!fwnode_has_op(fwnode, device_is_available))
>> + return true;
>> return fwnode_call_bool_op(fwnode, device_is_available);
>> }
>> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
>>
Hi Dan,
On Sat, Oct 24, 2020 at 05:33:32PM +0100, Dan Scally wrote:
> On 24/10/2020 15:29, Sakari Ailus wrote:
> > On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote:
> >> On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote:
> >>> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote:
> >>>> On 20/10/2020 13:06, Sakari Ailus wrote:
> >>>>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote:
> >>>>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote:
> >>>>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices
> >>>>>>> only; that status being determined through the .device_is_available() op
> >>>>>>> of the device's fwnode. As software_nodes don't have that operation and
> >>>>>>> adding it is meaningless, we instead need to check if the device's fwnode
> >>>>>>> is a software_node and if so pass the appropriate flag to disable that
> >>>>>>> check
> >>>>>>
> >>>>>> Period.
> >>>>>>
> >>>>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
> >>>>>
> >>>>> The device availability test is actually there for a reason. Some firmware
> >>>>> implementations put all the potential devices in the tables and only one
> >>>>> (of some) of them are available.
> >>>>>
> >>>>> Could this be implemented so that if the node is a software node, then get
> >>>>> its parent and then see if that is available?
> >>>>>
> >>>>> I guess that could be implemented in software node ops. Any opinions?
> >>>>
> >>>> Actually when considering the cio2 device, it seems that
> >>>> set_secondary_fwnode() actually overwrites the _primary_, given
> >>>> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases,
> >>>> this wouldn't work.
> >>>
> >>> Ouch. I wonder when this happens --- have you checked what's the primary
> >>> there? I guess it might be if it's a PCI device without the corresponding
> >>> ACPI device node?
> >>>
> >>> I remember you had an is_available implementation that just returned true
> >>> for software nodes in an early version of the set? I think it would still
> >>> be a lesser bad in this case.
> >>
> >> How about the following ?
> >
> > Looks good to me.
>
> If we're agreed on this (and it's fine by me too), do you want me to
> include it in the next set, or are you going to do it separately Laurent?
Feel free to include it in the next version, but I can send a patch if
you prefer.
> >> diff --git a/drivers/base/property.c b/drivers/base/property.c
> >> index 81bd01ed4042..ea44ba846299 100644
> >> --- a/drivers/base/property.c
> >> +++ b/drivers/base/property.c
> >> @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
> >> /**
> >> * fwnode_device_is_available - check if a device is available for use
> >> * @fwnode: Pointer to the fwnode of the device.
> >> + *
> >> + * For fwnode node types that don't implement the .device_is_available()
> >> + * operation, such as software nodes, this function returns true.
> >> */
> >> bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
> >> {
> >> + if (!fwnode_has_op(fwnode, device_is_available))
> >> + return true;
> >> return fwnode_call_bool_op(fwnode, device_is_available);
> >> }
> >> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
--
Regards,
Laurent Pinchart