| Message ID | 20180313085534.11650-2-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote: > The lists managing the device-links can be traversed to > find the link between two devices. The device_link_add() APIs > does traverse these lists to check if there's already a link > setup between the two devices. > So, add a new APIs, device_link_find(), to find an existing > device link between two devices - suppliers and consumers. > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > > * New patch added to this series. > > drivers/base/core.c | 30 +++++++++++++++++++++++++++--- > include/linux/device.h | 2 ++ > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 5847364f25d9..e8c9774e4ba2 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) > return 0; > } > > +/** > + * device_link_find - find any existing link between two devices. > + * @consumer: Consumer end of the link. > + * @supplier: Supplier end of the link. > + * > + * Returns pointer to the existing link between a supplier and > + * and consumer devices, or NULL if no link exists. > + */ > +struct device_link *device_link_find(struct device *consumer, > + struct device *supplier) > +{ > + struct device_link *link = NULL; > + > + if (!consumer || !supplier) > + return NULL; > + > + list_for_each_entry(link, &supplier->links.consumers, s_node) > + if (link->consumer == consumer) > + break; > + Any mutual exclusion? Or is the caller expected to take care of it? And if so, then how? > + return link; > +} > +EXPORT_SYMBOL_GPL(device_link_find); > + > /** > * device_link_add - Create a link between two devices. > * @consumer: Consumer end of the link. > @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device *consumer, > goto out; > } > > - list_for_each_entry(link, &supplier->links.consumers, s_node) > - if (link->consumer == consumer) > - goto out; > + link = device_link_find(consumer, supplier); > + if (link) > + goto out; > > link = kzalloc(sizeof(*link), GFP_KERNEL); > if (!link) > diff --git a/include/linux/device.h b/include/linux/device.h > index b093405ed525..13bc1884c3eb 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct device *dev); > struct device_link *device_link_add(struct device *consumer, > struct device *supplier, u32 flags); > void device_link_del(struct device_link *link); > +struct device_link *device_link_find(struct device *consumer, > + struct device *supplier); > > #ifdef CONFIG_PRINTK > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 3:10 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote: >> The lists managing the device-links can be traversed to >> find the link between two devices. The device_link_add() APIs >> does traverse these lists to check if there's already a link >> setup between the two devices. >> So, add a new APIs, device_link_find(), to find an existing >> device link between two devices - suppliers and consumers. >> >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> >> * New patch added to this series. >> >> drivers/base/core.c | 30 +++++++++++++++++++++++++++--- >> include/linux/device.h | 2 ++ >> 2 files changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index 5847364f25d9..e8c9774e4ba2 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) >> return 0; >> } >> >> +/** >> + * device_link_find - find any existing link between two devices. >> + * @consumer: Consumer end of the link. >> + * @supplier: Supplier end of the link. >> + * >> + * Returns pointer to the existing link between a supplier and >> + * and consumer devices, or NULL if no link exists. >> + */ >> +struct device_link *device_link_find(struct device *consumer, >> + struct device *supplier) >> +{ >> + struct device_link *link = NULL; >> + >> + if (!consumer || !supplier) >> + return NULL; >> + >> + list_for_each_entry(link, &supplier->links.consumers, s_node) >> + if (link->consumer == consumer) >> + break; >> + > > Any mutual exclusion? > > Or is the caller expected to take care of it? And if so, then how? I think it's better that we take care of lock here in the code rather than depending on the caller. But i can't take device_links_write_lock() since device_link_add() already takes that. regards Vivek > >> + return link; >> +} >> +EXPORT_SYMBOL_GPL(device_link_find); >> + >> /** >> * device_link_add - Create a link between two devices. >> * @consumer: Consumer end of the link. >> @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device *consumer, >> goto out; >> } >> >> - list_for_each_entry(link, &supplier->links.consumers, s_node) >> - if (link->consumer == consumer) >> - goto out; >> + link = device_link_find(consumer, supplier); >> + if (link) >> + goto out; >> >> link = kzalloc(sizeof(*link), GFP_KERNEL); >> if (!link) >> diff --git a/include/linux/device.h b/include/linux/device.h >> index b093405ed525..13bc1884c3eb 100644 >> --- a/include/linux/device.h >> +++ b/include/linux/device.h >> @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct device *dev); >> struct device_link *device_link_add(struct device *consumer, >> struct device *supplier, u32 flags); >> void device_link_del(struct device_link *link); >> +struct device_link *device_link_find(struct device *consumer, >> + struct device *supplier); >> >> #ifdef CONFIG_PRINTK >> >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 2:25 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > The lists managing the device-links can be traversed to > find the link between two devices. The device_link_add() APIs > does traverse these lists to check if there's already a link > setup between the two devices. > So, add a new APIs, device_link_find(), to find an existing > device link between two devices - suppliers and consumers. > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > > * New patch added to this series. > > drivers/base/core.c | 30 +++++++++++++++++++++++++++--- > include/linux/device.h | 2 ++ > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 5847364f25d9..e8c9774e4ba2 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) > return 0; > } > > +/** > + * device_link_find - find any existing link between two devices. > + * @consumer: Consumer end of the link. > + * @supplier: Supplier end of the link. > + * > + * Returns pointer to the existing link between a supplier and > + * and consumer devices, or NULL if no link exists. > + */ > +struct device_link *device_link_find(struct device *consumer, > + struct device *supplier) > +{ > + struct device_link *link = NULL; > + > + if (!consumer || !supplier) > + return NULL; > + > + list_for_each_entry(link, &supplier->links.consumers, s_node) > + if (link->consumer == consumer) > + break; > + > + return link; My bad, this too needs fixing (didn't add the changes to the patch :( ) list_for_each_entry(link, &supplier->links.consumers, s_node) if (link->consumer == consumer) return link; return NULL; > +} > +EXPORT_SYMBOL_GPL(device_link_find); > + > /** > * device_link_add - Create a link between two devices. > * @consumer: Consumer end of the link. > @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device *consumer, > goto out; > } > > - list_for_each_entry(link, &supplier->links.consumers, s_node) > - if (link->consumer == consumer) > - goto out; > + link = device_link_find(consumer, supplier); > + if (link) > + goto out; > > link = kzalloc(sizeof(*link), GFP_KERNEL); > if (!link) > diff --git a/include/linux/device.h b/include/linux/device.h > index b093405ed525..13bc1884c3eb 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct device *dev); > struct device_link *device_link_add(struct device *consumer, > struct device *supplier, u32 flags); > void device_link_del(struct device_link *link); > +struct device_link *device_link_find(struct device *consumer, > + struct device *supplier); > > #ifdef CONFIG_PRINTK > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vivek, Thanks for the patch. On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > The lists managing the device-links can be traversed to > find the link between two devices. The device_link_add() APIs > does traverse these lists to check if there's already a link > setup between the two devices. > So, add a new APIs, device_link_find(), to find an existing > device link between two devices - suppliers and consumers. I'm wondering if this API would be useful for anything else that the problem we're trying to solve with deleting links without storing them anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a better alternative? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: > Hi Vivek, > > Thanks for the patch. > > On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> The lists managing the device-links can be traversed to >> find the link between two devices. The device_link_add() APIs >> does traverse these lists to check if there's already a link >> setup between the two devices. >> So, add a new APIs, device_link_find(), to find an existing >> device link between two devices - suppliers and consumers. > > I'm wondering if this API would be useful for anything else that the > problem we're trying to solve with deleting links without storing them > anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a > better alternative? Yea, that sounds simpler i think. Will add this API instead of find_link(). Thanks. regards vivek > > Best regards, > Tomasz > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > Hi Tomasz, > > On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: >> Hi Vivek, >> >> Thanks for the patch. >> >> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >>> The lists managing the device-links can be traversed to >>> find the link between two devices. The device_link_add() APIs >>> does traverse these lists to check if there's already a link >>> setup between the two devices. >>> So, add a new APIs, device_link_find(), to find an existing >>> device link between two devices - suppliers and consumers. >> >> I'm wondering if this API would be useful for anything else that the >> problem we're trying to solve with deleting links without storing them >> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a >> better alternative? > > Yea, that sounds simpler i think. Will add this API instead of > find_link(). Thanks. Perhaps let's wait for a moment to see if there are other opinions. :) Rafael, Lucas, any thoughts? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/03/18 09:55, Vivek Gautam wrote: > On Tue, Mar 13, 2018 at 3:10 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote: >>> The lists managing the device-links can be traversed to >>> find the link between two devices. The device_link_add() APIs >>> does traverse these lists to check if there's already a link >>> setup between the two devices. >>> So, add a new APIs, device_link_find(), to find an existing >>> device link between two devices - suppliers and consumers. >>> >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> --- >>> >>> * New patch added to this series. >>> >>> drivers/base/core.c | 30 +++++++++++++++++++++++++++--- >>> include/linux/device.h | 2 ++ >>> 2 files changed, 29 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>> index 5847364f25d9..e8c9774e4ba2 100644 >>> --- a/drivers/base/core.c >>> +++ b/drivers/base/core.c >>> @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) >>> return 0; >>> } >>> >>> +/** >>> + * device_link_find - find any existing link between two devices. >>> + * @consumer: Consumer end of the link. >>> + * @supplier: Supplier end of the link. >>> + * >>> + * Returns pointer to the existing link between a supplier and >>> + * and consumer devices, or NULL if no link exists. >>> + */ >>> +struct device_link *device_link_find(struct device *consumer, >>> + struct device *supplier) >>> +{ >>> + struct device_link *link = NULL; >>> + >>> + if (!consumer || !supplier) >>> + return NULL; >>> + >>> + list_for_each_entry(link, &supplier->links.consumers, s_node) >>> + if (link->consumer == consumer) >>> + break; >>> + >> >> Any mutual exclusion? >> >> Or is the caller expected to take care of it? And if so, then how? > > I think it's better that we take care of lock here in the code rather > than depending > on the caller. > But i can't take device_links_write_lock() since device_link_add() > already takes that. Well, the normal pattern is to break out the internal helper function as-is, then add a public wrapper which validates inputs, handles locking, etc., equivalently to existing caller(s). See what device_link_del() and others do, e.g.: static struct device_link *__device_link_find(struct device *consumer, struct device *supplier) { list_for_each_entry(link, &supplier->links.consumers, s_node) if (link->consumer == consumer) return link; return NULL; } struct device_link *device_link_find(struct device *consumer, struct device *supplier) { struct device_link *link; if (!consumer || !supplier) return NULL; device_links_write_lock(); link = __device_link_find(consumer, supplier); device_links_write_unlock(); return link; } where device_link_add() would call __device_link_find() directly. However, as Tomasz points out (and I hadn't really considered), if the only reasonable thing to with a link once you've found it is to delete it, then in terms of the public API it may well make more sense to just implement something like a device_link_remove() which does both in one go. Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Robin, On Tue, Mar 13, 2018 at 6:19 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 13/03/18 09:55, Vivek Gautam wrote: >> >> On Tue, Mar 13, 2018 at 3:10 PM, Rafael J. Wysocki <rjw@rjwysocki.net> >> wrote: >>> >>> On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote: >>>> >>>> The lists managing the device-links can be traversed to >>>> find the link between two devices. The device_link_add() APIs >>>> does traverse these lists to check if there's already a link >>>> setup between the two devices. >>>> So, add a new APIs, device_link_find(), to find an existing >>>> device link between two devices - suppliers and consumers. >>>> >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> --- >>>> >>>> * New patch added to this series. >>>> >>>> drivers/base/core.c | 30 +++++++++++++++++++++++++++--- >>>> include/linux/device.h | 2 ++ >>>> 2 files changed, 29 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>>> index 5847364f25d9..e8c9774e4ba2 100644 >>>> --- a/drivers/base/core.c >>>> +++ b/drivers/base/core.c >>>> @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device >>>> *dev, void *not_used) >>>> return 0; >>>> } >>>> >>>> +/** >>>> + * device_link_find - find any existing link between two devices. >>>> + * @consumer: Consumer end of the link. >>>> + * @supplier: Supplier end of the link. >>>> + * >>>> + * Returns pointer to the existing link between a supplier and >>>> + * and consumer devices, or NULL if no link exists. >>>> + */ >>>> +struct device_link *device_link_find(struct device *consumer, >>>> + struct device *supplier) >>>> +{ >>>> + struct device_link *link = NULL; >>>> + >>>> + if (!consumer || !supplier) >>>> + return NULL; >>>> + >>>> + list_for_each_entry(link, &supplier->links.consumers, s_node) >>>> + if (link->consumer == consumer) >>>> + break; >>>> + >>> >>> >>> Any mutual exclusion? >>> >>> Or is the caller expected to take care of it? And if so, then how? >> >> >> I think it's better that we take care of lock here in the code rather >> than depending >> on the caller. >> But i can't take device_links_write_lock() since device_link_add() >> already takes that. > > > Well, the normal pattern is to break out the internal helper function as-is, > then add a public wrapper which validates inputs, handles locking, etc., > equivalently to existing caller(s). See what device_link_del() and others > do, e.g.: > > static struct device_link *__device_link_find(struct device *consumer, > struct device *supplier) > { > list_for_each_entry(link, &supplier->links.consumers, s_node) > if (link->consumer == consumer) > return link; > return NULL; > } > > struct device_link *device_link_find(struct device *consumer, > struct device *supplier) > { > struct device_link *link; > > if (!consumer || !supplier) > return NULL; > > device_links_write_lock(); > link = __device_link_find(consumer, supplier); > device_links_write_unlock(); > return link; > } > > where device_link_add() would call __device_link_find() directly. Right, I understand it now. Thanks for detailed explanation. regards Vivek > > However, as Tomasz points out (and I hadn't really considered), if the only > reasonable thing to with a link once you've found it is to delete it, then > in terms of the public API it may well make more sense to just implement > something like a device_link_remove() which does both in one go. > > Robin. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: > On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: > > Hi Tomasz, > > > > On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: > >> Hi Vivek, > >> > >> Thanks for the patch. > >> > >> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam > >> <vivek.gautam@codeaurora.org> wrote: > >>> The lists managing the device-links can be traversed to > >>> find the link between two devices. The device_link_add() APIs > >>> does traverse these lists to check if there's already a link > >>> setup between the two devices. > >>> So, add a new APIs, device_link_find(), to find an existing > >>> device link between two devices - suppliers and consumers. > >> > >> I'm wondering if this API would be useful for anything else that the > >> problem we're trying to solve with deleting links without storing them > >> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a > >> better alternative? > > > > Yea, that sounds simpler i think. Will add this API instead of > > find_link(). Thanks. > > Perhaps let's wait for a moment to see if there are other opinions. :) > > Rafael, Lucas, any thoughts? It is not clear to me what the device_link_del_dev(consumer, supplier) would do. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 14, 2018 at 8:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: >> On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >> > Hi Tomasz, >> > >> > On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: >> >> Hi Vivek, >> >> >> >> Thanks for the patch. >> >> >> >> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam >> >> <vivek.gautam@codeaurora.org> wrote: >> >>> The lists managing the device-links can be traversed to >> >>> find the link between two devices. The device_link_add() APIs >> >>> does traverse these lists to check if there's already a link >> >>> setup between the two devices. >> >>> So, add a new APIs, device_link_find(), to find an existing >> >>> device link between two devices - suppliers and consumers. >> >> >> >> I'm wondering if this API would be useful for anything else that the >> >> problem we're trying to solve with deleting links without storing them >> >> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a >> >> better alternative? >> > >> > Yea, that sounds simpler i think. Will add this API instead of >> > find_link(). Thanks. >> >> Perhaps let's wait for a moment to see if there are other opinions. :) >> >> Rafael, Lucas, any thoughts? > > It is not clear to me what the device_link_del_dev(consumer, supplier) would do. It would delete a link between consumer and supplier. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, March 14, 2018 12:50:54 PM CET Tomasz Figa wrote: > On Wed, Mar 14, 2018 at 8:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: > >> On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam > >> <vivek.gautam@codeaurora.org> wrote: > >> > Hi Tomasz, > >> > > >> > On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: > >> >> Hi Vivek, > >> >> > >> >> Thanks for the patch. > >> >> > >> >> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam > >> >> <vivek.gautam@codeaurora.org> wrote: > >> >>> The lists managing the device-links can be traversed to > >> >>> find the link between two devices. The device_link_add() APIs > >> >>> does traverse these lists to check if there's already a link > >> >>> setup between the two devices. > >> >>> So, add a new APIs, device_link_find(), to find an existing > >> >>> device link between two devices - suppliers and consumers. > >> >> > >> >> I'm wondering if this API would be useful for anything else that the > >> >> problem we're trying to solve with deleting links without storing them > >> >> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a > >> >> better alternative? > >> > > >> > Yea, that sounds simpler i think. Will add this API instead of > >> > find_link(). Thanks. > >> > >> Perhaps let's wait for a moment to see if there are other opinions. :) > >> > >> Rafael, Lucas, any thoughts? > > > > It is not clear to me what the device_link_del_dev(consumer, supplier) would do. > > It would delete a link between consumer and supplier. If there's one I suppose. I'm wondering if you are somehow trying to address the same problem as the device links reference counting patch from Lukas that has been queued up for 4.17 already. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, On 14/03/18 11:57, Rafael J. Wysocki wrote: > On Wednesday, March 14, 2018 12:50:54 PM CET Tomasz Figa wrote: >> On Wed, Mar 14, 2018 at 8:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: >>>> On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam >>>> <vivek.gautam@codeaurora.org> wrote: >>>>> Hi Tomasz, >>>>> >>>>> On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: >>>>>> Hi Vivek, >>>>>> >>>>>> Thanks for the patch. >>>>>> >>>>>> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam >>>>>> <vivek.gautam@codeaurora.org> wrote: >>>>>>> The lists managing the device-links can be traversed to >>>>>>> find the link between two devices. The device_link_add() APIs >>>>>>> does traverse these lists to check if there's already a link >>>>>>> setup between the two devices. >>>>>>> So, add a new APIs, device_link_find(), to find an existing >>>>>>> device link between two devices - suppliers and consumers. >>>>>> >>>>>> I'm wondering if this API would be useful for anything else that the >>>>>> problem we're trying to solve with deleting links without storing them >>>>>> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a >>>>>> better alternative? >>>>> >>>>> Yea, that sounds simpler i think. Will add this API instead of >>>>> find_link(). Thanks. >>>> >>>> Perhaps let's wait for a moment to see if there are other opinions. :) >>>> >>>> Rafael, Lucas, any thoughts? >>> >>> It is not clear to me what the device_link_del_dev(consumer, supplier) would do. >> >> It would delete a link between consumer and supplier. > > If there's one I suppose. > > I'm wondering if you are somehow trying to address the same problem as the > device links reference counting patch from Lukas that has been queued up for 4.17 > already. Not quite - the issue here is that we have one supplier with an arbitrarily large number of consumers, and would prefer that supplier not to have to spend a whole bunch of memory to store all the struct device_link pointers for the sole reason of having something to give to device_link_del() at the end, given that the device links code is already keeping track of everything internally anyway. The current API would permit doing this: iommu_attach(dev) { ... if (!device_link_add(dev, iommu, IOMMU_LINK_FLAGS)) return -ENODEV; ... } iommu_detach(dev) { ... // Will return the existing link from earlier link = device_link_add(dev, iommu, IOMMU_LINK_FLAGS); device_link_del(link); // Needed once refcounting is in place //device_link_del(link); ... } but it looks so wacky and non-obvious that we'd like to encapsulate the same behaviour into a more formal interface (my personal naming preference would be device_link_remove(consumer, supplier)). Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 14, 2018 at 12:12:05PM +0100, Rafael J. Wysocki wrote: > On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: > > On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > > On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: > > >> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > >>> The lists managing the device-links can be traversed to > > >>> find the link between two devices. The device_link_add() APIs > > >>> does traverse these lists to check if there's already a link > > >>> setup between the two devices. > > >>> So, add a new APIs, device_link_find(), to find an existing > > >>> device link between two devices - suppliers and consumers. > > >> > > >> I'm wondering if this API would be useful for anything else that the > > >> problem we're trying to solve with deleting links without storing them > > >> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a > > >> better alternative? > > > > > > Yea, that sounds simpler i think. Will add this API instead of > > > find_link(). Thanks. > > > > Perhaps let's wait for a moment to see if there are other opinions. :) > > > > Rafael, Lucas, any thoughts? > > It is not clear to me what the device_link_del_dev(consumer, supplier) > would do. The point appears to be that the pointer to the device_link need not be stored somewhere for later deletion. The newly added function would check if a device link exists and delete it if so. However I don't understand why storing the pointer would be a problem? Also, would using DL_FLAG_AUTOREMOVE avoid the need for the additional function? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 14, 2018 at 12:14:15PM +0000, Robin Murphy wrote: > >>On Wed, Mar 14, 2018 at 8:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >>>On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: > >>>>On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > >>>>>On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: > >>>>>>On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > >>>>>>>The lists managing the device-links can be traversed to > >>>>>>>find the link between two devices. The device_link_add() APIs > >>>>>>>does traverse these lists to check if there's already a link > >>>>>>>setup between the two devices. > >>>>>>>So, add a new APIs, device_link_find(), to find an existing > >>>>>>>device link between two devices - suppliers and consumers. > >>>>>> > >>>>>>I'm wondering if this API would be useful for anything else that the > >>>>>>problem we're trying to solve with deleting links without storing them > >>>>>>anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a > >>>>>>better alternative? > >>>>> > >>>>>Yea, that sounds simpler i think. Will add this API instead of > >>>>>find_link(). Thanks. > >>>> > >>>>Perhaps let's wait for a moment to see if there are other opinions. :) > >>>> > >>>>Rafael, Lucas, any thoughts? > >>> > >>>It is not clear to me what the device_link_del_dev(consumer, supplier) > >>>would do. > > Not quite - the issue here is that we have one supplier with an arbitrarily > large number of consumers, and would prefer that supplier not to have to > spend a whole bunch of memory to store all the struct device_link pointers > for the sole reason of having something to give to device_link_del() at the > end, given that the device links code is already keeping track of everything > internally anyway. Makes sense to me. How about an additional flag which autoremoves the link on provider unbind? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lukasz, On 3/14/2018 5:57 PM, Lukas Wunner wrote: > On Wed, Mar 14, 2018 at 12:14:15PM +0000, Robin Murphy wrote: >>>> On Wed, Mar 14, 2018 at 8:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>>> On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: >>>>>> On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: >>>>>>> On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa <tfiga@chromium.org> wrote: >>>>>>>> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: >>>>>>>>> The lists managing the device-links can be traversed to >>>>>>>>> find the link between two devices. The device_link_add() APIs >>>>>>>>> does traverse these lists to check if there's already a link >>>>>>>>> setup between the two devices. >>>>>>>>> So, add a new APIs, device_link_find(), to find an existing >>>>>>>>> device link between two devices - suppliers and consumers. >>>>>>>> I'm wondering if this API would be useful for anything else that the >>>>>>>> problem we're trying to solve with deleting links without storing them >>>>>>>> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a >>>>>>>> better alternative? >>>>>>> Yea, that sounds simpler i think. Will add this API instead of >>>>>>> find_link(). Thanks. >>>>>> Perhaps let's wait for a moment to see if there are other opinions. :) >>>>>> >>>>>> Rafael, Lucas, any thoughts? >>>>> It is not clear to me what the device_link_del_dev(consumer, supplier) >>>>> would do. >> Not quite - the issue here is that we have one supplier with an arbitrarily >> large number of consumers, and would prefer that supplier not to have to >> spend a whole bunch of memory to store all the struct device_link pointers >> for the sole reason of having something to give to device_link_del() at the >> end, given that the device links code is already keeping track of everything >> internally anyway. > Makes sense to me. How about an additional flag which autoremoves the > link on provider unbind? If I understand this correctly, if we create the device link with DL_FLAG_AUTOREMOVE, the link is deleted after a consumer unbind. During a supplier unbind all we get is a WARN_ON with DL_FLAG_AUTOREMOVE. I guess that's an intended behavior? If this is the case, then the consumer/supplier drivers just don't have to take care of deleting the device link explicitly. Is my understanding correct? regards Vivek > > Thanks, > > Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/core.c b/drivers/base/core.c index 5847364f25d9..e8c9774e4ba2 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) return 0; } +/** + * device_link_find - find any existing link between two devices. + * @consumer: Consumer end of the link. + * @supplier: Supplier end of the link. + * + * Returns pointer to the existing link between a supplier and + * and consumer devices, or NULL if no link exists. + */ +struct device_link *device_link_find(struct device *consumer, + struct device *supplier) +{ + struct device_link *link = NULL; + + if (!consumer || !supplier) + return NULL; + + list_for_each_entry(link, &supplier->links.consumers, s_node) + if (link->consumer == consumer) + break; + + return link; +} +EXPORT_SYMBOL_GPL(device_link_find); + /** * device_link_add - Create a link between two devices. * @consumer: Consumer end of the link. @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device *consumer, goto out; } - list_for_each_entry(link, &supplier->links.consumers, s_node) - if (link->consumer == consumer) - goto out; + link = device_link_find(consumer, supplier); + if (link) + goto out; link = kzalloc(sizeof(*link), GFP_KERNEL); if (!link) diff --git a/include/linux/device.h b/include/linux/device.h index b093405ed525..13bc1884c3eb 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct device *dev); struct device_link *device_link_add(struct device *consumer, struct device *supplier, u32 flags); void device_link_del(struct device_link *link); +struct device_link *device_link_find(struct device *consumer, + struct device *supplier); #ifdef CONFIG_PRINTK
The lists managing the device-links can be traversed to find the link between two devices. The device_link_add() APIs does traverse these lists to check if there's already a link setup between the two devices. So, add a new APIs, device_link_find(), to find an existing device link between two devices - suppliers and consumers. Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- * New patch added to this series. drivers/base/core.c | 30 +++++++++++++++++++++++++++--- include/linux/device.h | 2 ++ 2 files changed, 29 insertions(+), 3 deletions(-)