Re: [PATCH] cdx: add MSI support for CDX bus

[Thomas Gleixner <tglx@linutronix.de>]

Nipun!

On Fri, May 12 2023 at 19:50, Nipun Gupta wrote:
> On 5/11/2023 3:59 AM, Thomas Gleixner wrote:
>> CDX is not any different than PCI. The actual "interrupt chip" is not
>> part of the bus, it's part of the device and pretending that it is a bus
>> specific thing is just running in to the same cul-de-sac sooner than
>> later.
>
> I understand your viewpoint, but would state that CDX bus is somewhat 
> different than PCI in the sense that firmware is a controller for
> all the devices and their configuration. CDX bus controller sends all 
> the write_msi_msg commands to firmware running on RPU over the RPmsg and 
> it is the firmware which interfaces with actual devices to pass this 
> information to devices in a way agreed between firmware and device. The 
> only way to pass MSI information to device is via firmware and CDX bus 
> controller is only entity which can communicate with the firmware for
> this.

Fair enough, but we wouldn't had this dicussion if the above information
would have been part of the changelog. See?

>> IIRC, there is a gap vs. interrupt affinity setting from user space,
>> which is irrelevant for I2C, SPI etc. configured interrupt chips as they
>> raise interrupt via an SoC interrupt pin and that's the entity which
>> does the affinity management w/o requiring I2C/SPI. IIRC I posted a
>> patch snippet to that effect in one of those lengthy PCI/MSI/IMS threads
>> because that is also required for MSI storage which happens to be in
>> queue memory and needs to be synchronized via some command channel. But
>> I can't be bothered to search for it as it's a no-brainer to fix that
>> up.
>
> Thanks for this analysis and pointing the hidden crucial issues with the 
> implementation. These needs to be fixed.
>
> As per your suggestion, we can add Firmware interaction code in the
> irq_bus_sync_xx APIs. Another option is to change the
> cdx_mcdi_rpc_async() API to atomic synchronous API.

I'm not a great fan of that. Depending on how long this update takes the
CPU will busy wait for it to complete with interrupts disabled and locks
held.

> We are evaluating both the solutions and will update the
> implementation accordingly.

Thanks,
 
         tglx