Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Niklas Cassel <cassel@kernel.org>]

On Mon, May 12, 2025 at 01:00:02PM +0530, Manivannan Sadhasivam wrote:
> On Sat, May 10, 2025 at 01:43:39PM +0200, Niklas Cassel wrote:
> > On Sat, May 10, 2025 at 11:27:55AM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Apr 30, 2025 at 02:31:59PM +0200, Niklas Cassel wrote:
> > > > While the parameter 'interrupts' to the functions pci_epc_set_msi() and
> > > > pci_epc_set_msix() represent the actual number of interrupts, and
> > > > pci_epc_get_msi() and pci_epc_get_msix() return the actual number of
> > > > interrupts.
> > > > 
> > > > These endpoint library functions just mentioned will however supply
> > > > "interrupts - 1" to the EPC callback functions pci_epc_ops->set_msi() and
> > > > pci_epc_ops->set_msix(), and likewise add 1 to return value from
> > > > pci_epc_ops->get_msi() and pci_epc_ops->get_msix(),
> > > 
> > > Only {get/set}_msix() callbacks were having this behavior, right?
> > 
> > pci_epc_get_msix() adds 1 to the return of epc->ops->get_msix().
> > pci_epc_set_msix() subtracts 1 to the parameter sent to epc->ops->set_msix().
> > 
> > pci_epc_get_msi() does 1 << interrupt from the return of epc->ops->get_msi().
> > So a return of 0 from epc->ops->get_msi() will result in pci_epc_get_msi()
> > returning 1. A return of 1 from epc->ops->get_msi() will result in
> > pci_epc_get_msi() returning 2.
> > 
> > Similar for pci_epc_set_msi(). It will call order_base_2() on the parameter
> > before sending it to epc->ops->set_msi().
> > 
> 
> Yeah. I was pointing out the fact that bitshifting != incrementing/decrementing
> 1. And you just mentioned the latter for both msi/msix callbacks. I can ammend
> it while applying.

Thank you!



> > I am a bit worried about patches after the cleanup getting backported, which
> > will need to be different before and after this cleanup.
> 
> We can add stable+noautosel to mark the patches to not backport.
> 
> > Perhaps renaming the
> > callbacks at the same as the cleanup might be a good idea?
> > 
> > It should be a simple cleanup though, just do the order_base_2() etc in the
> > driver callbacks themselves.
> > 
> > We really should rename the parameter of .set_msi() though, as it is totally
> > misleading as of now.
> > 
> 
> As I said above, we should keep the semantics for 'interrupt' and make changes
> accordingly i.e., by doing order_base_2() inside the callbacks as you suggested.

Yeah, I agree, better to rename the parameter of the existing callbacks that use
mmc/mme to interrupts, and actually perform the cleanup so that the take interrupts
instead of mmc/mme.


Considering that it is only 4 drivers, with a maximum of 4 callbacks in each driver
that needs to be cleaned up, it's not so bad.

Since we seem to have identified all the weirdness with the existing APIs,
let me try to cleanup this mess this week.


(Would be nice to get this fixed queued first though, so cleanup patches can
come on top.)


Kind regards,
Niklas