drivers/amba/bus.c | 130 ++++++++++++++++++++++++++--------------------- include/linux/amba/bus.h | 1 - 2 files changed, 72 insertions(+), 59 deletions(-)
This patch series is to make amba_match(), as bus_type @amba_bustype's
match(), also follow below ideal rule:
bus_type's match() should only return bool type compatible integer 0 or
1 ideally since its main operations are lookup and comparison normally.
Which has been followed by match() of all other bus_types in current
kernel tree.
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (3):
amba: bus: Warn on adding an AMBA device without valid periphid
amba: bus: Move empty @amba_proxy_drv's definition to the front
amba: bus: Move reading periphid operation from amba_match() to amba_probe()
drivers/amba/bus.c | 130 ++++++++++++++++++++++++++---------------------
include/linux/amba/bus.h | 1 -
2 files changed, 72 insertions(+), 59 deletions(-)
---
base-commit: 888f67e621dda5c2804a696524e28d0ca4cf0a80
change-id: 20240829-fix_amba-0f09aa1fc3b1
Best regards,
--
Zijun Hu <quic_zijuhu@quicinc.com>
On Mon, Sep 09, 2024 at 07:37:31AM +0800, Zijun Hu wrote: > This patch series is to make amba_match(), as bus_type @amba_bustype's > match(), also follow below ideal rule: > > bus_type's match() should only return bool type compatible integer 0 or > 1 ideally since its main operations are lookup and comparison normally. > > Which has been followed by match() of all other bus_types in current > kernel tree. How does this work with e.g. udev module loading? If the ID isn't known until we attempt to probe a device, then if all AMBA drivers are modular, there'll be no drivers registered to cause an attempt to match a device to a driver, and thus there will be no peripheral IDs for udev to use to load modules. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 2024/9/17 17:52, Russell King (Oracle) wrote: > On Mon, Sep 09, 2024 at 07:37:31AM +0800, Zijun Hu wrote: >> This patch series is to make amba_match(), as bus_type @amba_bustype's >> match(), also follow below ideal rule: >> >> bus_type's match() should only return bool type compatible integer 0 or >> 1 ideally since its main operations are lookup and comparison normally. >> >> Which has been followed by match() of all other bus_types in current >> kernel tree. > > How does this work with e.g. udev module loading? If the ID isn't > known until we attempt to probe a device, then if all AMBA drivers > are modular, there'll be no drivers registered to cause an attempt > to match a device to a driver, and thus there will be no > peripheral IDs for udev to use to load modules. > drivers/amba/bus.c have registered a empty AMBA driver @amba_proxy_drv this change makes AMBA device without valid periphid ONLY match with the empty driver, that will trigger trying to read periphid with bus_type's @amba_bustype's amba_probe(). it will send AMBA device adding uevent once periphid is read out, then udev will notice the uevent and perform further actions.
On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote: > > This patch series is to make amba_match(), as bus_type @amba_bustype's > match(), also follow below ideal rule: > > bus_type's match() should only return bool type compatible integer 0 or > 1 ideally since its main operations are lookup and comparison normally. > > Which has been followed by match() of all other bus_types in current > kernel tree. The intent or need for this patch series is completely unclear. The code you are moving around was also pretty delicate and hard to get right. Without a much better description for why we need this, I'd like to give this a NACK. Also, patch 3/3 is not at all easy to understand and seems to be doing way more than what the commit message is trying to do. -Saravana > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > Zijun Hu (3): > amba: bus: Warn on adding an AMBA device without valid periphid > amba: bus: Move empty @amba_proxy_drv's definition to the front > amba: bus: Move reading periphid operation from amba_match() to amba_probe() > > drivers/amba/bus.c | 130 ++++++++++++++++++++++++++--------------------- > include/linux/amba/bus.h | 1 - > 2 files changed, 72 insertions(+), 59 deletions(-) > --- > base-commit: 888f67e621dda5c2804a696524e28d0ca4cf0a80 > change-id: 20240829-fix_amba-0f09aa1fc3b1 > > Best regards, > -- > Zijun Hu <quic_zijuhu@quicinc.com> >
On 2024/9/9 15:24, Saravana Kannan wrote:
> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>> match(), also follow below ideal rule:
>>
>> bus_type's match() should only return bool type compatible integer 0 or
>> 1 ideally since its main operations are lookup and comparison normally.
>>
>> Which has been followed by match() of all other bus_types in current
>> kernel tree.
>
> The intent or need for this patch series is completely unclear. The
> code you are moving around was also pretty delicate and hard to get
> right.
>
> Without a much better description for why we need this, I'd like to
> give this a NACK.
>
> Also, patch 3/3 is not at all easy to understand and seems to be doing
> way more than what the commit message is trying to do.
>
thanks for your code review.
let me explain the issue here firstly to go on with discussion, will
correct it by next revision.
amba_match(), as bus_type @amba_bustype's match(), operate hardware to
read id, may return -EPROBE_DEFER consequently.
this design is not very good and has several disadvantages shown below:
1) it is not good time to operate hardware in a bus_type's match().
hardware is not ready to operate normally in a bus_type's match()
as driver_probe_device() shown, there are still many preparations
to make hardware to operate after a bus_type's match(), for example,
resuming device and its ancestors, ensuring all its suppliers have
drivers bound, activating its power domain, ...
2) it should not operate hardware in a bus_type's match().
a bus_type's match() will obviously be triggered frequently, and
hardware operation is slow normally, it will reduce efficiency for
device attaching driver if operate hardware in a bus_type's match().
a bus_type's match() will become not reentry for a device and driver
if operating hardware is failed but can't recover initial hardware state.
3) for driver_attach(), a bus_type's match() are called without
device_lock(dev) firstly, it often causes concurrent issue when
operate hardware within a bus_type's match(), look at below AMBA related
fix:
Commit: 25af7406df59 ("ARM: 9229/1: amba: Fix use-after-free in
amba_read_periphid()")
which introduce an extra @periphid_lock to fix this issue.
4) it may not be proper for a bus_type's match() to return -EPROBE_DEFER
which will stop driver API bus_rescan_devices() from scanning other
remaining devices, that is not expected as discussed by below thread:
https://lore.kernel.org/all/20240904-bus_match_unlikely-v1-3-122318285261@quicinc.com/
5) amba_match() is the only bus_type's match which breaks below ideal
rule in current kernel tree:
bus_type's match() should only return bool type compatible integer 0
or 1 ideally since its main operations are lookup and comparison normally.
Our purpose is to solve this issue then enforce the ideal rule mentioned
in 5).
so we send this patch series to start a topic about how to solve this
issue (^^).
> -Saravana
>
On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2024/9/9 15:24, Saravana Kannan wrote:
> > On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>
> >> This patch series is to make amba_match(), as bus_type @amba_bustype's
> >> match(), also follow below ideal rule:
> >>
> >> bus_type's match() should only return bool type compatible integer 0 or
> >> 1 ideally since its main operations are lookup and comparison normally.
> >>
> >> Which has been followed by match() of all other bus_types in current
> >> kernel tree.
> >
> > The intent or need for this patch series is completely unclear. The
> > code you are moving around was also pretty delicate and hard to get
> > right.
> >
> > Without a much better description for why we need this, I'd like to
> > give this a NACK.
> >
> > Also, patch 3/3 is not at all easy to understand and seems to be doing
> > way more than what the commit message is trying to do.
> >
>
> thanks for your code review.
>
> let me explain the issue here firstly to go on with discussion, will
> correct it by next revision.
>
> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
> read id, may return -EPROBE_DEFER consequently.
>
> this design is not very good and has several disadvantages shown below:
>
> 1) it is not good time to operate hardware in a bus_type's match().
> hardware is not ready to operate normally in a bus_type's match()
> as driver_probe_device() shown, there are still many preparations
> to make hardware to operate after a bus_type's match(), for example,
> resuming device and its ancestors, ensuring all its suppliers have
> drivers bound, activating its power domain, ...
>
> 2) it should not operate hardware in a bus_type's match().
> a bus_type's match() will obviously be triggered frequently, and
> hardware operation is slow normally, it will reduce efficiency for
> device attaching driver if operate hardware in a bus_type's match().
>
> a bus_type's match() will become not reentry for a device and driver
> if operating hardware is failed but can't recover initial hardware state.
>
> 3) for driver_attach(), a bus_type's match() are called without
> device_lock(dev) firstly, it often causes concurrent issue when
> operate hardware within a bus_type's match(), look at below AMBA related
> fix:
> Commit: 25af7406df59 ("ARM: 9229/1: amba: Fix use-after-free in
> amba_read_periphid()")
> which introduce an extra @periphid_lock to fix this issue.
>
> 4) it may not be proper for a bus_type's match() to return -EPROBE_DEFER
> which will stop driver API bus_rescan_devices() from scanning other
> remaining devices, that is not expected as discussed by below thread:
>
> https://lore.kernel.org/all/20240904-bus_match_unlikely-v1-3-122318285261@quicinc.com/
>
> 5) amba_match() is the only bus_type's match which breaks below ideal
> rule in current kernel tree:
> bus_type's match() should only return bool type compatible integer 0
> or 1 ideally since its main operations are lookup and comparison normally.
All of this used to happen even if the bus match wasn't doing what
it's doing today. You don't seem to have full context on how amba
devices are added and probed. What you see now is a clean
up/simplification of how things used to work.
Please go read this patch history and git log history for these files
to get more context.
Nack for the entire series. It'll never go in.
-Saravana
>
>
> Our purpose is to solve this issue then enforce the ideal rule mentioned
> in 5).
>
> so we send this patch series to start a topic about how to solve this
> issue (^^).
>
> > -Saravana
> >
>
>
On 2024/9/11 00:27, Saravana Kannan wrote:
> On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> On 2024/9/9 15:24, Saravana Kannan wrote:
>>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>>>
>>>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>>>> match(), also follow below ideal rule:
>>>>
>>> Also, patch 3/3 is not at all easy to understand and seems to be doing
>>> way more than what the commit message is trying to do.
>>>
>>
>> thanks for your code review.
>>
>> let me explain the issue here firstly to go on with discussion, will
>> correct it by next revision.
>>
>> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
>> read id, may return -EPROBE_DEFER consequently.
>>
>> this design is not very good and has several disadvantages shown below:
>>
>> 1) it is not good time to operate hardware in a bus_type's match().
>> hardware is not ready to operate normally in a bus_type's match()
>> as driver_probe_device() shown, there are still many preparations
>> to make hardware to operate after a bus_type's match(), for example,
>> resuming device and its ancestors, ensuring all its suppliers have
>> drivers bound, activating its power domain, ...
>>
.....
>> 5) amba_match() is the only bus_type's match which breaks below ideal
>> rule in current kernel tree:
>> bus_type's match() should only return bool type compatible integer 0
>> or 1 ideally since its main operations are lookup and comparison normally.
>
> All of this used to happen even if the bus match wasn't doing what
> it's doing today. You don't seem to have full context on how amba
> devices are added and probed. What you see now is a clean
> up/simplification of how things used to work.
>
> Please go read this patch history and git log history for these files
> to get more context.
>
> Nack for the entire series. It'll never go in.
>
sorry, not agree with you.
IMO, it is easy to make amba_match() return bool type as shown below:
make amba_match() always match with AMBA device with INvalid periphid
and move reading id operation into amba_dma_configure().
Above solution can have the same logical as current one but it looks ugly.
so i make below optimizations to get this patch series:
1) only make AMBA device with INvalid periphid match with existing empty
amba_proxy_drv to reduce unnecessary reading id operation.
2) moving reading id operation to amba_probe() looks more graceful.
Look at below 3 consecutive history commits:
git log --pretty='%h (\"%s\")' 656b8035b0ee -3
Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
bus_type.match()")
Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
gets returned on positive match")
Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
value gets returned on positive match")
the first AMBA related commit breaks that a bus_type's match() have bool
type return value.
the remaining two commits at the same time really do not like negative
return value for a bus_type's match().
thanks
> -Saravana
>
>>
>>
>> Our purpose is to solve this issue then enforce the ideal rule mentioned
>> in 5).
>>
>> so we send this patch series to start a topic about how to solve this
>> issue (^^).
>>
>>> -Saravana
>>>
>>
>>
On Wed, Sep 11, 2024 at 5:51 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2024/9/11 00:27, Saravana Kannan wrote:
> > On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>
> >> On 2024/9/9 15:24, Saravana Kannan wrote:
> >>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>>>
> >>>> This patch series is to make amba_match(), as bus_type @amba_bustype's
> >>>> match(), also follow below ideal rule:
> >>>>
> >>> Also, patch 3/3 is not at all easy to understand and seems to be doing
> >>> way more than what the commit message is trying to do.
> >>>
> >>
> >> thanks for your code review.
> >>
> >> let me explain the issue here firstly to go on with discussion, will
> >> correct it by next revision.
> >>
> >> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
> >> read id, may return -EPROBE_DEFER consequently.
> >>
> >> this design is not very good and has several disadvantages shown below:
> >>
> >> 1) it is not good time to operate hardware in a bus_type's match().
> >> hardware is not ready to operate normally in a bus_type's match()
> >> as driver_probe_device() shown, there are still many preparations
> >> to make hardware to operate after a bus_type's match(), for example,
> >> resuming device and its ancestors, ensuring all its suppliers have
> >> drivers bound, activating its power domain, ...
> >>
> .....
>
> >> 5) amba_match() is the only bus_type's match which breaks below ideal
> >> rule in current kernel tree:
> >> bus_type's match() should only return bool type compatible integer 0
> >> or 1 ideally since its main operations are lookup and comparison normally.
> >
> > All of this used to happen even if the bus match wasn't doing what
> > it's doing today. You don't seem to have full context on how amba
> > devices are added and probed. What you see now is a clean
> > up/simplification of how things used to work.
> >
> > Please go read this patch history and git log history for these files
> > to get more context.
> >
> > Nack for the entire series. It'll never go in.
> >
>
> sorry, not agree with you.
>
> IMO, it is easy to make amba_match() return bool type as shown below:
>
> make amba_match() always match with AMBA device with INvalid periphid
> and move reading id operation into amba_dma_configure().
>
> Above solution can have the same logical as current one but it looks ugly.
>
> so i make below optimizations to get this patch series:
>
> 1) only make AMBA device with INvalid periphid match with existing empty
> amba_proxy_drv to reduce unnecessary reading id operation.
No it doesn't. Once match() returns -EPROBE_DEFER we don't try
matching with other drivers. So it doesn't cause more reads.
> 2) moving reading id operation to amba_probe() looks more graceful.
To do a driver/device match, you need to periphid. It doesn't make
sense to push that into some stub probe function instead of doing it
where it's needed. In the match function.
> Look at below 3 consecutive history commits:
>
> git log --pretty='%h (\"%s\")' 656b8035b0ee -3
> Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
> bus_type.match()")
> Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
> gets returned on positive match")
> Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
> value gets returned on positive match")
Those are commits from 2016! Way before any of the cleanup was done.
> the first AMBA related commit breaks that a bus_type's match() have bool
> type return value.
Have you actually looked at the definition of match and it's doc? It's
return type is int and not bool. And the doc says it should return
-EPROBE_DEFER.
> the remaining two commits at the same time really do not like negative
> return value for a bus_type's match().
This whole series is fixing a non-issue because you have a subjective
opinion that the reading of periphid should happen outside of the
match() function where it's actually needed.
And you even have a comment saying it's adding a race.
Russell,
Definite huge NACK from me. Please don't merge this series. I don't
see it fixing anything and it's moving around code for
pointless/questionable reasons. If it is fixing any real bug, I've yet
to hear it explained properly.
If I don't reply further, it means my NACK stands. If the replies
somehow convince me to remove my NACK, I'll do so.
-Saravana
On 2024/10/3 09:49, Saravana Kannan wrote:
> On Wed, Sep 11, 2024 at 5:51 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> On 2024/9/11 00:27, Saravana Kannan wrote:
>>> On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>>>>
>>>> On 2024/9/9 15:24, Saravana Kannan wrote:
>>>>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>>>>>
>>>>>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>>>>>> match(), also follow below ideal rule:
>>>>>>
>>>>> Also, patch 3/3 is not at all easy to understand and seems to be doing
>>>>> way more than what the commit message is trying to do.
>>>>>
>>>>
>>>> thanks for your code review.
>>>>
>>>> let me explain the issue here firstly to go on with discussion, will
>>>> correct it by next revision.
>>>>
>>>> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
>>>> read id, may return -EPROBE_DEFER consequently.
>>>>
>>>> this design is not very good and has several disadvantages shown below:
>>>>
>>>> 1) it is not good time to operate hardware in a bus_type's match().
>>>> hardware is not ready to operate normally in a bus_type's match()
>>>> as driver_probe_device() shown, there are still many preparations
>>>> to make hardware to operate after a bus_type's match(), for example,
>>>> resuming device and its ancestors, ensuring all its suppliers have
>>>> drivers bound, activating its power domain, ...
>>>>
>> .....
>>
>>>> 5) amba_match() is the only bus_type's match which breaks below ideal
>>>> rule in current kernel tree:
>>>> bus_type's match() should only return bool type compatible integer 0
>>>> or 1 ideally since its main operations are lookup and comparison normally.
>>>
>>> All of this used to happen even if the bus match wasn't doing what
>>> it's doing today. You don't seem to have full context on how amba
>>> devices are added and probed. What you see now is a clean
>>> up/simplification of how things used to work.
>>>
>>> Please go read this patch history and git log history for these files
>>> to get more context.
>>>
>>> Nack for the entire series. It'll never go in.
>>>
>>
>> sorry, not agree with you.
>>
>> IMO, it is easy to make amba_match() return bool type as shown below:
>>
>> make amba_match() always match with AMBA device with INvalid periphid
>> and move reading id operation into amba_dma_configure().
>>
>> Above solution can have the same logical as current one but it looks ugly.
>>
>> so i make below optimizations to get this patch series:
>>
>> 1) only make AMBA device with INvalid periphid match with existing empty
>> amba_proxy_drv to reduce unnecessary reading id operation.
>
> No it doesn't. Once match() returns -EPROBE_DEFER we don't try
> matching with other drivers. So it doesn't cause more reads.
>
sorry to give reply late due to travel.
above points is not applicable for driver attaching as explained below.
devn_n : AMBA device n with periphid n
drvn : AMBA device devn_n's driver.
AMBA bus
├── dev0_0 // dev0 with Invalid periphid 0
├── @amba_proxy_drv // the empty AMBA driver
now let us register 2 AMBA drivers drv1 and drv2.
-EPROBE_DEFER returned during trying to match dev0_0 with drv1
can NOT stopping reading periphid when trying to match dev0_0 with
drv2 since the error code is ignored for driver attaching.
MY solution is shown below:
1) only make device with Invalid ID 0 match with @amba_proxy_drv
this will reduce unnecessary reading ID operations.
2) Move reading ID from bus's match() to bus's probe()
>> 2) moving reading id operation to amba_probe() looks more graceful.
>
> To do a driver/device match, you need to periphid. It doesn't make
> sense to push that into some stub probe function instead of doing it
> where it's needed. In the match function.
>
1) it is not good location for a bus's match() to operate hardware to
read ID as explained by below link:
https://lore.kernel.org/all/a4cf15fb-bbaa-4ed0-a1d5-c362b7a5c6e2@icloud.com/
2) ideally, amba_proxy_drv's probe() is better than the AMBA bus's
probe() to read ID, i use the later since it is simpler and my limited
AMBA knowledge.
@amba_proxy_drv was introduced to ensure triggering reading ID operation
, perhaps, make it take charge for reading ID and have similar role as
the generic USB driver.
>> Look at below 3 consecutive history commits:
>>
>> git log --pretty='%h (\"%s\")' 656b8035b0ee -3
>> Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
>> bus_type.match()")
>> Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
>> gets returned on positive match")
>> Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
>> value gets returned on positive match")
>
> Those are commits from 2016! Way before any of the cleanup was done.
>
>> the first AMBA related commit breaks that a bus_type's match() have bool
>> type return value.
>
> Have you actually looked at the definition of match and it's doc? It's
> return type is int and not bool. And the doc says it should return
> -EPROBE_DEFER.
>
yes, but ALL other bus's match()s only return 0 and 1, AMBA is the only
one which returns extra -EPROBE_DEFER.
-EPROBE_DEFER is a probe related error code and should not be returned
by a bus's match()
it was below AMBA change which make a bus's match() return -EPROBE_DEFER.
Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
bus_type.match()")
>> the remaining two commits at the same time really do not like negative
>> return value for a bus_type's match().
>
> This whole series is fixing a non-issue because you have a subjective
> opinion that the reading of periphid should happen outside of the
> match() function where it's actually needed.
>
this patch serials is to improve design and not to fix bugs.
> And you even have a comment saying it's adding a race.
as explained by 3) of below link, we don't need extra periphid_lock
any more if moving reading ID operations into probe(). and relevant
a bit race is acceptable.
https://lore.kernel.org/all/a4cf15fb-bbaa-4ed0-a1d5-c362b7a5c6e2@icloud.com/
>
> Russell,
>
> Definite huge NACK from me. Please don't merge this series. I don't
> see it fixing anything and it's moving around code for
> pointless/questionable reasons. If it is fixing any real bug, I've yet
> to hear it explained properly.
>
> If I don't reply further, it means my NACK stands. If the replies
> somehow convince me to remove my NACK, I'll do so.
>
> -Saravana
© 2016 - 2024 Red Hat, Inc.