Re: [PATCH v4 2/6] PCI: spacemit-k1: Add multiple PHY handles support

[Inochi Amaoto]

On Sat, Jul 11, 2026 at 03:44:10PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 10, 2026 at 06:55:10PM +0800, Inochi Amaoto wrote:
> > On Fri, Jul 10, 2026 at 11:07:40AM +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 10, 2026 at 09:57:05AM +0800, Inochi Amaoto wrote:
> > > > On Thu, Jul 09, 2026 at 10:16:28AM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Jul 09, 2026 at 12:00:22PM +0800, Inochi Amaoto wrote:
> 
> ...
> 
> > > > > > +	k1->phy_count = i;
> > > > > > +	if (k1->phy_count == 0)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	return 0;
> > > > > 
> > > > > This doesn't seem correct to me, I would expect phy_count to be assigned only
> > > > > when it's valid. (Yes, perhaps 0 is the same as it was, but semantically it's
> > > > > different 0 in this case.)
> > > > 
> > > > I guess you think 0 is a valid number? I can not understand what you thing
> > > > Assign this to 0 if there is no phy is fine to me, which shows there is 0
> > > > vaild phy found.
> > > 
> > > Isn't it already 0? Semantically code is wrong in a flow (not in the result).
> > 
> > In fact it is already 0 here. But I am not understand why you thing is wrong.
> > Could you explain it in detail? (Maybe you think it is not good to return 
> > -EINVAL?)
> 
> You rewrite 0 by 0, but the fact of the rewriting is inaccuracy.
> We should not rewrite the default (whatever it is) with 0 count
> as semantically they are different cases. The rule of thumb, we
> don't assign values in case of errors, we leave them as is and
> it's user / caller responsibility to assign the default and handle
> errors properly. This is simple layering violation.
> 
> Your code should be
> 
> 	if (i == 0)
> 		return -EINVAL;
> 
> 	k1->phy_count = i;
> 	return 0;
> 

Good, I understand it. Thanks.


> > > > > See also above. Do we have some PHY API that just counts provided PHYs?
> > > > > If not, that what you should probably add first, before this patch.
> > > > 
> > > > I have not found any api for this. But the actual problem is, how the api
> > > > is designed. I have checked both the array bulk api for reset and clock,
> > > > it seems like it is much more than this patch...
> > > 
> > > Yeah, I looked at the phy-core and I think it will be hard to implement.
> > > So, the idea is then is to reallocate the pointer each time you get a new PHY.
> > > In this case the phy_count will reflect the actual memory consumption by phy.
> > 
> > Emmm, I think it is kind of buggy and not necessary. In most case
> > this array is not long actually, so allocate some pointer should be
> > fine and be an acceptable cost.
> 
> Then the counted_by will be incorrect as it may access valid memory, but
> unused by the driver.
> 

After some search I found a requirement in GCC patch:
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/653123.html

It seems like the array can have more elements than the counter.
Some I guess the reallocation is not necessary and the counter
is still correct. Correct me if I am wrong.

> ...
> 
> > > > > > +	for (i = 0; i < k1->phy_count; i++)
> > > > > 
> > > > > 	for (unsigned int i = 0; i < k1->phy_count; i++)
> > > > > 
> > > > 
> > > > I agree with the unsigned int, but I guess this definition is not
> > > > allowed in linux.
> > > 
> > > It's allowed and it's encouraged even by Linus. As long as iterator is local,
> > > use this syntax sugar and reduce its scope. It hardens the code.
> > 
> > Could you give me a reference url to check,
> 
> Sure, there are two (one for integers and one for pointers)
> https://lore.kernel.org/lkml/CAHk-=wiCOTW5UftUrAnvJkr6769D29tF7Of79gUjdQHS_TkF5A@xxxxxxxxxxxxxx/
> https://lore.kernel.org/lkml/CAHk-=wgy8p4is8ApEQCT5NS7XFb+NXeo-TKz7jRRZVksLLBSrQ@xxxxxxxxxxxxxx/
> 
> > I have not found this on the coding-style.
> > https://www.kernel.org/doc/html/latest/process/coding-style.html
> 
> Feel free to update the documentation.
> 

Good to know thanks.

> > > > > > +		phy_exit(k1->phy[i]);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Regards,
Inochi