Re: [RFC PATCH 00/11] Refactor MSI to support Non-PCI device

[Yijing Wang <wangyijing@huawei.com>]

On 2014/8/1 21:52, Arnd Bergmann wrote:
> On Wednesday 30 July 2014, Yijing Wang wrote:
>> On 2014/7/29 22:08, Arnd Bergmann wrote:
>>> On Saturday 26 July 2014 11:08:37 Yijing Wang wrote:
>>>>
>>>> The new data struct for generic MSI driver.
>>>> struct msi_irqs {
>>>>         u8 msi_enabled:1; /* Enable flag */
>>>>         u8 msix_enabled:1;
>>>>         struct list_head msi_list; /* MSI desc list */
>>>>         void *data;     /* help to find the MSI device */
>>>>         struct msi_ops *ops; /* MSI device specific hook */
>>>> };
>>>> struct msi_irqs is used to manage MSI related informations. Every device supports
>>>> MSI should contain this data struct and allocate it.
>>>
>>> I think you should have a stronger association with the 'struct
>>> device' here. Can you replace the 'void *data' with 'struct device *dev'?
>>
>> Actually, I used the struct device *dev in my first draft, finally, I replaced
>> it with void *data, because some MSI devices don't have a struct device *dev,
>> like the existing hpet device, dmar msi device, and OF device, like the ARM consolidator.
>>
>> Of course, we can make the MSI devices have their own struct device, and register to
>> device tree, eg, add a class device named MSI_DEV. But I'm not sure whether it is appropriate.
> 
> It doesn't have to be in the (OF) device tree, but I think it absolutely makes
> sense to use the 'struct device' infrastructure here, as almost everything uses
> a device, and the ones that don't do that today can be easily changed.

I will try to use "struct device" infrastructure, thanks for your suggestion. :)

> 
>>> The other part I'm not completely sure about is how you want to
>>> have MSIs map into normal IRQ descriptors. At the moment, all
>>> MSI users are based on IRQ numbers, but this has known scalability problems.
>>
>> Hmmm, I still use the IRQ number to map the MSIs to IRQ description.
>> I'm sorry, I don't understand you meaning.
>> What are the scalability problems you mentioned ?
>> For device drivers, they always process interrupt in two steps.
>> If irq is the legacy interrupt, drivers will first
>> use the irq_of_parse_and_map() or pci_enable_device() to parse and get the IRQ number.
>> Then drivers will call the request_irq() to register the interrupt handler.
>> If irq is MSIs, first call pci_enable_msi/x() to get the IRQ number and then call
>> request_irq() to register interrupt handler.
> 
> The method you describe here makes sense for PCI devices that are required to support
> legacy interrupts and may or may not support MSI on a given system, but not so much
> for platform devices for which we know exactly whether we want to use MSI
> or legacy interrupts.
> 
> In particular if you have a device that can only do MSI, the entire pci_enable_msi
> step is pointless: all we need to do is program the correct MSI target address/message
> pair into the device and register the handler.

Yes, I almost agree if we won't change the existing hundreds of drivers, what
I worried about is some drivers may want to know the IRQ numbers, and use the IRQ
number to process different things, as I mentioned in another reply.
But we can also provide the interface which integrate MSI configuration and request_irq(),
if most drivers don't care the IRQ number.

> 
>>> I wonder if we can do the interface in a way that
>>> hides the interrupt number from generic device drivers and just
>>> passes a 'struct irq_desc'. Note that there are long-term plans to
>>> get rid of IRQ numbers entirely, but those plans have existed for
>>> a long time already without anybody seriously addressing the device
>>> driver interfaces so far, so it might never really happen.
>>>
>>
>> Maybe this is a huge work, now hundreds drivers use the IRQ number, so maybe we can consider
>> this in a separate title.
> 
> Sorry for being unclear here: I did suggest changing all drivers now. What I meant
> is that we use a different API for non-PCI devices that works without IRQ numbers.
> I don't think we should touch the PCI interfaces at this point.

OK, I got it.

>>> What I'd envision as the API from the device driver perspective is something
>>> as simple like this:
>>>
>>> struct msi_desc *msi_request(struct msi_chip *chip, irq_handler_t handler,
>>> 			unsigned long flags, const char *name, struct device *dev);
>>>
>>> which would get an msi descriptor that is valid for this device (dev)
>>> connected to a particular msi_chip, and associate a handler function
>>> with it. The device driver can call that function and retrieve the
>>> address/message pair from the msi_desc in order to store it in its own
>>> device specific registers. The request_irq() can be handled internally
>>> to msi_request().
>>
>> This is a huge change for device drivers, and some device drivers don't know which msi_chip
>> their MSI irq deliver to. I'm reworking the msi_chip, and try to use msi_chip to eliminate
>> all arch_msi_xxx() under every arch in kernel. And the important point is how to create the
>> binding for the MSI device to the target msi_chip.
> 
> Which drivers are you thinking of? Again, I wouldn't expect to change any PCI drivers,
> but only platform drivers that do native MSI, so we only have to change drivers that
> do not support any MSI at all yet and that need to be changed anyway in order to add
> support.

I mean platform device drivers, because we can find the target msi_chip by some platform
interfaces(like the existing of_pci_find_msi_chip_by_node()). So we no need to explicitly provide
the msi_chip as the function argument.

> 
>> For PCI device, some arm platform already bound the msi_chip to the pci hostbridge, then all
>> pci devices under the pci hostbridge deliver their MSI irqs to the target msi_chip.
>> And other platform create the binding in DTS file, then the MSI device can find their msi_chip
>> by device_node.
>> I don't know whether there are other situations, we should provide a generic interface that
>> every MSI device under every platform can use it to find its msi_chip exactly.
> 
> We have introduced the "msi-parent" property to mirror the "interrupt-parent" property.
> For the PCI case, this property is only needed in the PCI host controller, and there
> can be a system-wide default, by putting the "msi-parent" property into the root device
> node or the node of a bus that is parent to all devices supporting MSI.
> 
> For non-PCI devices, it should be possible to override the "msi-parent" property per
> device, but those can also use the global property.
> 
> The main use case that I see are PCI host controllers that have their own MSI catcher
> included, so meaning that any PCI device can either send its MSIs there, or to a
> system-wide GICv3 instance, and we need a way to select which one.

Yes, agree.

> 
> A degenerate case of this would be a system where a PCI device sends its MSI into
> the host controller, that generates a legacy interrupt and that in turn gets 
> sent to an irqchip which turns it back into an MSI for the GICv3. This would of
> course be very inefficient, but I think we should be able to express this with
> both the binding and the in-kernel framework just to be on the safe side.

Yes, the best way to tell the kernel which msi_chip should deliver to is describe
the binding in DTS file. If a real degenerate case found, we can update the platform
interface which is responsible for getting the match msi_chip in future.

> 
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> .
> 


-- 
Thanks!
Yijing