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

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2014/7/30 10:45, Yijing Wang wrote:
> On 2014/7/29 22:08, Arnd Bergmann wrote:
>> On Saturday 26 July 2014 11:08:37 Yijing Wang wrote:
>>>         The series is a draft of generic MSI driver that supports PCI
>>> and Non-PCI device which have MSI capability. If you're not interested
>>> it, sorry for the noise.
>>
>> I've finally managed to take some time to look at the series. Overall,
>> the concept looks good to me, and the patches look very well implemented.
>>
>> The part I'm not sure about is the interface we want to end up with
>> at the end of the series. More on that below
> 
> Hi Arnd,
>    Thanks for your review and comments very much!
> Please refer the inline comments.
> 
>>
>>> The series is based on Linux-3.16-rc1.
>>>
>>> MSI was introduced in PCI Spec 2.2. Currently, kernel MSI 
>>> driver codes are bonding with PCI device. Because MSI has a lot
>>> advantages in design. More and more non-PCI devices want to
>>> use MSI as their default interrupt. The existing MSI device
>>> include HPET. HPET driver provide its own MSI code to initialize
>>> and process MSI interrupts. In the latest GIC v3 spec, legacy device
>>> can deliver MSI by the help of a relay device named consolidator.
>>> Consolidator can translate the legacy interrupts connected to it
>>> to MSI/MSI-X. And new non-PCI device will be designed to 
>>> support MSI in future. So make the MSI driver code be generic will 
>>> help the non-PCI device use MSI more simply.
>>>
>>> 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.
> 
>>
>> 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 ?
We have soft limitation of nr_irqs or hard limitation NR_IRQS,
we couldn't allocate as much irq number as we need in some cases,
such as to support MSI-x.

> 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.
> 
>> 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.
> 
>>> struct msi_ops {
>>>         struct msi_desc *(*msi_setup_entry)(struct msi_irqs *msi, struct msi_desc *entry);
>>>         int msix_setup_entries(struct msi_irqs *msi, struct msix_entry *entries);
>>>         u32 (*msi_mask_irq)(struct msi_desc *desc, u32 mask, u32 flag);
>>>         u32 (*msix_mask_irq)(struct msi_desc *desc, u32 flag);
>>>         void (*msi_read_message)(struct msi_desc *desc, struct msi_msg *msg);
>>>         void (*msi_write_message)(struct msi_desc *desc, struct msi_msg *msg);
>>>         void (*msi_set_intx)(struct msi_irqs *msi, int enable);
>>> };
>>> struct msi_ops provides several hook functions, generic MSI driver will call
>>> the hook functions to access device specific registers. PCI devices will share
>>> the same msi_ops, because they have the same way to access MSI hardware registers.
>>>
>>> Generic MSI layer export msi_capability_init() and msix_capability_init() functions
>>> to drivers. msi/x_capability_init() will initialize MSI capability data struct msi_desc
>>> and alloc the irq, then write the msi address/data value to hardware registers.
>>>
>>> This series only did compile test, we will test it in x86 and arm platform later.
>>
>> For the generic drivers, I don't see much point in differentiating between
>> MSI and MSI-X, as I believe the difference is something internal to the PCI
>> implementation.
> 
> Yes, we can integrate them, and use a generic ops, add a type in hook function to
> differentiate them.
> 
>>
>> With the other operations, I think they should all take a 'struct device *'
>> as the first argument for convenience and consistency. I don't think you actually
>> need msi_read_message(), and we could avoid msi_write_message() by doing it
>> the other way round.
>>
> 
> There only two functions use the read_msi_msg(), because every msi_desc has
> a struct msi_msg, and it caches the msi address and data. I will consider to
> retrieve the msg from cached msi_msg, then we can avoid the msi_read_message().
> But msi_write_message() maybe necessary, some xxx_set_affinity() functions and
> restore functions need the msi_write_message() to rewrite the address and data.
> 
>> 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.
> 
> 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.
> 
> 
> Thanks!
> Yijing.
> 
>>
>> .
>>
> 
>