lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 10 Jan 2024 01:20:40 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, 
	Leong Ching Swee <leong.ching.swee@...el.com>
Cc: Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Giuseppe Cavallaro <peppe.cavallaro@...com>, 
	linux-stm32@...md-mailman.stormreply.com, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org, devicetree@...r.kernel.org, 
	Rohan G Thomas <rohan.g.thomas@...el.com>
Subject: Re: [PATCH net-next v2 1/4] dt-bindings: net: snps,dwmac: per
 channel irq

On Tue, Jan 09, 2024 at 10:10:37AM +0100, Krzysztof Kozlowski wrote:
> On 07/01/2024 21:10, Serge Semin wrote:
> > On Fri, Jan 05, 2024 at 03:09:22PM +0800, Leong Ching Swee wrote:
> >> From: Swee Leong Ching <leong.ching.swee@...el.com>
> >>
> >> Add dt-bindings for per channel irq.
> >>
> >> Signed-off-by: Rohan G Thomas <rohan.g.thomas@...el.com>
> >> Signed-off-by: Swee Leong Ching <leong.ching.swee@...el.com>
> >> ---
> >>  .../devicetree/bindings/net/snps,dwmac.yaml   | 24 +++++++++++++------
> >>  1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> >> index 5c2769dc689a..e72dded824f4 100644
> >> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> >> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> >> @@ -103,17 +103,27 @@ properties:
> >>  
> >>    interrupts:
> >>      minItems: 1
> >> -    items:
> >> -      - description: Combined signal for various interrupt events
> >> -      - description: The interrupt to manage the remote wake-up packet detection
> >> -      - description: The interrupt that occurs when Rx exits the LPI state
> >> +    maxItems: 19
> >>  
> >>    interrupt-names:
> >>      minItems: 1
> >> +    maxItems: 19
> >>      items:
> >> -      - const: macirq
> >> -      - enum: [eth_wake_irq, eth_lpi]
> >> -      - const: eth_lpi
> >> +      oneOf:
> >> +        - description: Combined signal for various interrupt events
> >> +          const: macirq
> >> +        - description: The interrupt to manage the remote wake-up packet detection
> >> +          const: eth_wake_irq
> >> +        - description: The interrupt that occurs when Rx exits the LPI state
> >> +          const: eth_lpi
> >> +        - description: DMA Tx per-channel interrupt
> >> +          pattern: '^dma_tx[0-7]?$'
> >> +        - description: DMA Rx per-channel interrupt
> >> +          pattern: '^dma_rx[0-7]?$'
> >> +
> >> +    allOf:
> >> +      - contains:
> >> +          const: macirq
> > 
> > In order to restore the v1 discussion around this change, here is my
> > comment copied from there:
> > 
> >> As Rob correctly noted it's also better to make sure that 'macirq' is placed first
> >> in the array. So instead of the constraint above I guess the next one would
> >> make sure both the array has 'macirq' name and it's the first item:
> >>
> >> allOf:
> >>   - maxItems: 34
> >>     items:
> >>       - const: macirq
> > 
> > Leong said it didn't work:
> > https://lore.kernel.org/netdev/CH0PR11MB54904615B45E521DE6B1A7B3CF61A@CH0PR11MB5490.namprd11.prod.outlook.com/
> > 
> > Rob, Krzysztof, Conor could you please clarify whether this change is ok the
> > way it is or it would be better to preserve the stricter constraint
> > and fix the DT-schema validation tool somehow?
> 

> First of all this change is not good, because commit msg explains
> absolutely nothing why this is done and what exactly you want to achieve
> here. The "what" part often is obvious from the code, but not in this
> case. Are the per-channel IRQs conflicting with macirq or others? Are
> they complementary (maxItems: 19 suggests that, though, but could be
> mistake as well)? Do they affect all snps,dwmac derivatives or only some?
> 
> So many questions and zero answers in one liner commit msg!

Right. The commit message is way too modest =) Leong?

> 
> Now about the problem, I think we should preserve the order, assuming
> that these are complementary so first three must be defined.

Ok. But please note that "Wake" and "LPI" IRQs are optional. It's
possible to have a device with the "MAC" and "DMA" IRQs and no
individual "Wake"/"LPI" IRQ lines. Thus the only mandatory IRQ is
"MAC" which order (being always first), I agree, should be preserved.

> This
> however could be done in the device schema referencing snps,dwmac. I
> think I will repeat myself: I dislike this schema, because it mixes two
> purposes: defining shared part and defining final device part. The code
> in this patch is fine for a schema defining the shared part.
> 
> Therefore before we start growing this monstrosity into bigger one, I
> think we should go back to the plans of reworking and cleaning it.

If you are talking about the changes like introduced here (essentially
it's Patch 4):
https://www.spinics.net/lists/netdev/msg888079.html
I can resurrect it (rebase on the latest kernel, fix the notes, run
dt-validation, etc) and submit for review on the next week or so.
Then the Leong' patch in subject either won't be necessary or will
concern the shared schema only. Does it sound acceptable?

-Serge(y)

> 
> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ