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

[Nipun Gupta]

On 5/10/2023 3:31 AM, Thomas Gleixner wrote:
> Nipun!
>
> On Tue, May 09 2023 at 11:06, Nipun Gupta wrote:
> > > -----Original Message-----
> > > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Sent: Tuesday, May 9, 2023 1:32 PM
> > > To: Gupta, Nipun <Nipun.Gupta@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> > > maz@xxxxxxxxxx; jgg@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>
> Can you please fix your mail client to not copy half of the mail header
> into your reply?

Sure. Got it fixed.

> > > Caution: This message originated from an External Source. Use proper
> > > caution when opening attachments, clicking links, or responding.
>
> That's also relevant information for me, right?

Sorry to submit with this text, have already contacted concerned 
internal team regarding removal of this text. Have removed it manually 
for now.

> > > The only real CDX specific functionality here is a CDX specific
> > > irq_write_msi_msg() callback, right?
> > >
> > > And I gave you a pointer how this should be handled, but instead of
> > > helping this effort along you go off and implement it differently just
> > > because. Sigh!
> >
> > As you rightly mentioned the irq_chip has only irq_write_msi_msg() as
> > callback, but there is also cdx_msi_prepare() in msi_domain_ops which
> > needs to fetch device ID from CDX device, due to which we are currently
> > using separate CDX domain.
>
> Sure. But where is that information in the changelog?
>
> > IIUC, as per your suggestion we should have CDX bus token added into
> > its_init_dev_msi_info() of
> > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git/tree/drivers/irqchip/irq-gic-v3-its-msi-parent.c?h=devmsi-arm,
> > and register CDX specific 'msi_prepare' here; so that we can use
> > msi_create_device_irq_domain() to create a per device domain?
>
> Correct.
>
> I'm not insisting on that, but you could at least have had the courtesy
> of responding to my review reply and explain to me why you want to solve
> it differently and why my suggestion is not the right solution.
>
> Alternatively you could have added that information in the changelog or
> cover letter.
>
> So in summary you ignored _all_ review comments I made, went off and did
> something different and provided a slightly different useless changelog
> with the extra add on of a broken Signed-off-by chain.
>
> Feel free to ignore my reviews and the documentation which we put out
> there to make collaboration feasible for both sides, but please don't be
> upset when I ignore you and your patches in return.

Sincere apology for not responding to the earlier comments. Intention 
was never to ignore the review comments. Appreciate your vast changes 
regarding the MSI, and the patch series you shared took time to 
understand (provided other things as well), and it was quite late to 
reply. I understand that even in this case atleast I should have added 
this as part of the cover-letter.

IMHO, use-case for MSI in CDX subsystem is a bit different from per 
device MSI domain. Here we are trying to create a domain per CDX 
controller which is attached to a MSI controller, and all devices on a 
particular CDX controller will have same mechanism of write MSI message. 
Also, the current CDX controller that we have added has a different 
mechanism for MSI prepare (it gets requester ID from firmware).

In your opinion is there any advantage in moving to a per device domain 
for CDX devices? We can definitely rethink the implementation of MSI in 
CDX subsystem.

Thanks,
Nipun

> Thanks,
>
>          tglx