RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver

[Pawel Laszczak]

>
>Hi,
>
>Pawel Laszczak <pawell@xxxxxxxxxxx> writes:
>>>> IRQF_ONESHOT can be used  only in threaded handled.
>>>> "
>>>>  * IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished.
>>>>  *                Used by threaded interrupts which need to keep the
>>>>  *                irq line disabled until the threaded handler has been run.
>>>> "
>>>
>>>so?
>>
>> I don't understand why If I don't have threaded handler why I need IRQF_ONESHOT.
>> Why interrupt cannot be reenabled after hardirq handler finished ?
>> I do not use threaded handler so this flag seem unnecessary.
>
>Unless this has changed over the years, it was a requirement from IRQ susbystem.
>
>	/*
>	 * Drivers are often written to work w/o knowledge about the
>	 * underlying irq chip implementation, so a request for a
>	 * threaded irq without a primary hard irq context handler
>	 * requires the ONESHOT flag to be set. Some irq chips like
>	 * MSI based interrupts are per se one shot safe. Check the
>	 * chip flags, so we can avoid the unmask dance at the end of
>	 * the threaded handler for those.
>	 */
>	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
>		new->flags &= ~IRQF_ONESHOT;

>From description I understand that it should be set when driver uses only 
threaded handler without hard irq handler.
eg. 

	ret = devm_request_threaded_irq(dev, data->usb_id_irq,
					NULL, int3496_thread_isr,
					IRQF_SHARED | IRQF_ONESHOT |
					IRQF_TRIGGER_RISING |
					IRQF_TRIGGER_FALLING,
					dev_name(dev), data);

It make sense, we don't have hard irq handler so we can't clear source of interrupt. 
If we clear it immediately in interrupt controller then the same interrupt could 
be raised again, because it was not cleared e.g in controller register. 


>>>>>> +	} else {
>>>>>> +		struct usb_request *request;
>>>>>> +
>>>>>> +		if (priv_dev->eps[index]->flags & EP_WEDGE) {
>>>>>> +			cdns3_select_ep(priv_dev, 0x00);
>>>>>> +			return 0;
>>>>>> +		}
>>>>>> +
>>>>>> +		cdns3_dbg(priv_ep->cdns3_dev, "Clear Stalled endpoint %s\n",
>>>>>> +			  priv_ep->name);
>>>>>
>>>>>why do you need your own wrapper around dev_dbg()? This looks quite unnecessary.
>>>>
>>>> It's generic function used for adding message to trace.log.  It's not wrapper to dev_dbg
>>>>
>>>> void cdns3_dbg(struct cdns3_device *priv_dev, const char *fmt, ...)
>>>> {
>>>> 	struct va_format vaf;
>>>> 	va_list args;
>>>>
>>>> 	va_start(args, fmt);
>>>> 	vaf.fmt = fmt;
>>>> 	vaf.va = &args;
>>>> 	trace_cdns3_log(priv_dev, &vaf);
>>>> 	va_end(args);
>>>> }
>>>
>>>oh. Don't do it like that. Add a proper trace event that actually
>>>decodes the information you want. These random messages will give you
>>>trouble in the future. I had this sort of construct in dwc3 for a while
>>>and it became clear that it's a bad idea. It's best to have trace events
>>>that decode information coming from the HW. That way your trace logs
>>>have a "predictable" shape/format and you can easily find problem areas.
>>
>> Ok , I will change this.
>> I used such solution because I didn't want to create to many trace events.
>> I used it only for rely used messages.
>
>If you have these messages that are *really* needed, then you should add
>a trace event for it. Look at the events we have on dwc3 if you need
>some inspiration. Could also look at the history of our trace events to
>figure out how things changed over time.
>
>cheers
>
>--
>balbi