Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport

[Krishna Kurapati PSSNV]

On 7/14/2023 4:10 PM, Krishna Kurapati PSSNV wrote:
> On 7/14/2023 2:35 PM, Johan Hovold wrote:
> > On Wed, Jul 12, 2023 at 11:56:33PM +0530, Krishna Kurapati PSSNV wrote:
> > > On 7/12/2023 5:42 PM, Johan Hovold wrote:
> > > > On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote:
> > > > > Add support to read Multiport IRQ's related to quad port controller
> > > > > of SA8295 Device.
> > > > >
> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > > > > ---
> > > > >    drivers/usb/dwc3/dwc3-qcom.c | 108 
> > > > > +++++++++++++++++++++++++++++------
> > > > >    1 file changed, 91 insertions(+), 17 deletions(-)
> > > >
> > > > > +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev)
> > > > > +{
> > > > > +    struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> > > > > +    char irq_name[15];
> > > >
> > > > The interrupt device-name string can not be allocated on the stack or
> > > > reused as it is stored directly in each irqaction structure.
> > > >
> > > > This can otherwise lead to random crashes when accessing
> > > > /proc/interrupts:
> > > >
> > > >     https://lore.kernel.org/lkml/ZK6IV_jJPICX5r53@xxxxxxxxxxxxxxxxxxxx/
> >
> > >     Sure, will create a static array of names if possible in global
> > > section of file and use it to read interrupts.
> >
> > Or just use devm_kasprintf(), which should allow for a cleaner
> > implementation.
> >
> > I've fixed it up like this for my X13s wip branches:
> >
> >     https://github.com/jhovold/linux/commit/0898b54456bc2f4bd4d420480db98e6758771ace
> > >     Are you fine with seperating out setup_irq and setup_mp_irq 
> > > functions
> > > ? Can you please review comments and suggestion on [1].
> >
> > I haven't had time to look at your latest replies yet, but as I already
> > said when reviewing v9, it seems you should be using a common helper for
> > non-mp and mp.
> Hi Johan,
>
>   The gist of my mail was to see if I can defer qcom probe when dwc3 
> probe fails/or doesn't happen on of_plat_pop (which is logical) so that 
> we can move setup_irq to after dwc3_register_core so that we know 
> whether we are MP capable or not. This would help us move all IRQ 
> reading into one function.
Hi Johan,

 I see it is difficult to write a common helper. To do so, we need to 
know whether the device is MP capable or not in advance. And since it is 
not possible to know it before of_plat_pop is done, I see only few ways 
to do it:

1. Based on qcom node compatible string, I can read whether the device 
is MP capable or not and get IRQ's accordingly.

2. Read the port_info in advance but it needs me to go through some DT 
props and try getting this info. Or read xhci regs like we are doing in 
core (which is not good). Also since some Dt props can be missing, is it 
difficult to get the MP capability info before of_plat_pop is done.

3. Remove IRQ handling completely. Just because the device has IRQ's 
present, I don't see a point in adding them to bindings, and because we 
added them to bindings, we are making a patch to read them (and since 
this is a little challenging, the whole of multiport series is blocked 
although I don't need wakeup support on these interrupts right away).

Can't we let the rest of the patches go through and let interrupt 
handling for 2nd, 3rd and 4rth ports be taken care later ? I am asking 
this because I want the rest of the patches which are in good shape now 
(after fixing the nits mentioned) to get merged atleast. I will make 
sure to add interrupt handling later in a different series once this is 
merged once I send v10.

Or if there is a simpler way to do it, I would be happy to take any 
suggestions and complete this missing part in this series itself.

Regards,
Krishna,