linux-kernel - Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

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]

Message-ID: <CAPTae5+j9N=RBpfHVE-As+dz7HzrxXAH1enWrmSdFzu6DuaTBA@mail.gmail.com>
Date: Wed, 5 Feb 2025 01:07:53 -0800
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Thinh.Nguyen@...opsys.com, gregkh@...uxfoundation.org, 
	felipe.balbi@...ux.intel.com, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, johnyoun@...opsys.com, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, jameswei@...gle.com, 
	stable@...nel.org
Subject: Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

.

On Sun, Feb 2, 2025 at 6:11 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On Sun, Feb 02, 2025 at 03:50:59AM +0000, Badhri Jagan Sridharan wrote:
> > This change adds `snps,device-mode-intrpt-mod-interval`
>
> Thank you for your patch. There is something to discuss/improve.

Hi Krzysztof,

Thanks for taking the time to review ! Happy to address them.

>
> > which allows enabling interrupt moderation through
> > snps,dwc3 node.
> >
> > `snps,device-mode-intrpt-mod-interval`specifies the
> > minimum inter-interrupt interval in 250ns increments
> > during device mode operation. A value of 0 disables
> > the interrupt throttling logic and interrupts are
> > generated immediately if event count becomes non-zero.
> > Otherwise, the interrupt is signaled when all of the
> > following conditons are met which are, EVNT_HANDLER_BUSY
> > is 0, event count is non-zero and at least 250ns increments
> > of this value has elapsed since the last time interrupt
> > was de-asserted.
>
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Ack! will do in V2 of this patch.

>
> >
> > Cc: stable@...nel.org
> > Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
>
> I don't understand what are you fixing here.  Above commit does not
> introduce that property.

Although the above commit does not add this property, it has
implemented the entire feature except for the property so thought
sending this with "Fixes:" while CCing  stable@ will allow the
backport.  I am interested in having this patch in older kernel
versions as well where imod support has been added. Wondering what
would be the right way to achieve this. Eager to know your thoughts !

>
>
> > Signed-off-by: Badhri Jagan Sridharan <badhri@...gle.com>
> > ---
> >  .../devicetree/bindings/usb/snps,dwc3-common.yaml   | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> > index c956053fd036..3957f1dac3c4 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> > @@ -375,6 +375,19 @@ properties:
> >      items:
> >        enum: [1, 4, 8, 16, 32, 64, 128, 256]
> >
> > +  snps,device-mode-intrpt-mod-interval:
> > +    description:
> > +      Specifies the minimum inter-interrupt interval in 250ns increments
>
> Then use proper property unit suffix.

Ack ! changing to snps,device-mode-intrpt-mod-interval-ns in V2.

>
> > +      during device mode operation. A value of 0 disables the interrupt
> > +      throttling logic and interrupts are generated immediately if event
> > +      count becomes non-zero. Otherwise, the interrupt is signaled when
> > +      all of the following conditons are met which are, EVNT_HANDLER_BUSY
> > +      is 0, event count is non-zero and at least 250ns increments of this
> > +      value has elapsed since the last time interrupt was de-asserted.
>
> Why is this property of a board? Why different boards would wait
> different amount of time?

Interrupt moderation allows batch processing of events reported by the
controller.
A very low value of snps,device-mode-intrpt-mod-interval-ns implies
that the controller will interrupt more often to make the host
processor process a smaller set of events Vs a larger value will wake
up the host processor at longer intervals to process events (likely
more). So depending what the board is designed for this can be tuned
to achieve the needed outcome.

This is very similar to the "imod-interval-ns" in
https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/devicetree/bindings/usb/usb-xhci.yaml
except that in this case the Synopsys DWC3 controller supports this
for the device mode operation as well.

>
> > +    $ref: /schemas/types.yaml#/definitions/uint16
>
> Drop, use proper name suffix.

Ack, will drop in V2.

Thanks,
Badhri

>
> Best regards,
> Krzysztof
>