Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

On 2024/10/3 09:49, Saravana Kannan wrote:
> On Wed, Sep 11, 2024 at 5:51 AM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>>
>> On 2024/9/11 00:27, Saravana Kannan wrote:
>>> On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>>>>
>>>> On 2024/9/9 15:24, Saravana Kannan wrote:
>>>>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@xxxxxxxxxx> 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@xxxxxxxxxx/

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@xxxxxxxxxx/

> 
> 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