by Jiang Liu

[permalink] [raw]

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

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.
>
>>
>> .
>>
>
>

2014-07-30 07:20:50

by Yijing Wang

[permalink] [raw]

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

On 2014/7/30 14:47, Jiang Liu wrote:
>
>
> 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.

Oh, yes, this is a potential issue. Gerry, thanks for you explanation. :)

>
>> 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.
>>
>>>
>>> .
>>>
>>
>>
>
> .
>

--
Thanks!
Yijing