lkml.org 
[lkml]   [2023]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] cdx: add MSI support for CDX bus
From


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@linutronix.de>
>>> Sent: Tuesday, May 9, 2023 1:32 PM
>>> To: Gupta, Nipun <Nipun.Gupta@amd.com>; gregkh@linuxfoundation.org;
>>> maz@kernel.org; jgg@ziepe.ca; linux-kernel@vger.kernel.org
>
> 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
>
>

\
 
 \ /
  Last update: 2023-05-10 16:05    [W:0.173 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site