From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yijing Wang Subject: Re: [RFC PATCH 00/11] Refactor MSI to support Non-PCI device Date: Tue, 5 Aug 2014 10:12:01 +0800 Message-ID: <53E03D71.5080800@huawei.com> References: <1406344128-27055-1-git-send-email-wangyijing@huawei.com> <201408011552.32135.arnd@arndb.de> <53DF2B7B.4000605@huawei.com> <201408041659.31364.arnd@arndb.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <201408041659.31364.arnd@arndb.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Arnd Bergmann Cc: linux-arch@vger.kernel.org, Wuyun , Russell King , Paul.Mundt@huawei.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, "James E.J. Bottomley" , Xinwei Hu , Hanjun Guo , Bjorn Helgaas , virtualization@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org List-Id: linux-arch.vger.kernel.org >>> 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. > > The driver would still have the option of getting the IRQ number for now: With > the interface I imagine, you would get a 'struct msi_desc' pointer, from which > you can look up the 'struct irq_desc' pointer (either embedded in msi_desc, > or using a pointer from a member of msi_desc), and you can already get the > interrupt number from the irq_desc. > > My point was that a well-written driver already does not care about the interrupt > number: the only information a driver needs in the interrupt handler is a pointer > to its own context, which we already derive from the irq_desc. Agree, I will try to introduce this similar interface in next version, thanks! > > The main interface that currently requires the irq number is free_irq(), but > I would argue that we can just add a wrapper that takes the msi_desc pointer > as its first argument so the driver does not have to worry about it. > > We can add additional wrappers like that as needed. OK >>>> 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. > > Right, that works too. I was thinking we might need an interface that allows us to > pick a particular msi_chip if there are several alternatives (e.g. one in the GIC > and one in the PCI host), but you are right: we should normally be able to hardwire > that information in DT or elsewhere, and just need the 'struct device pointer' which > should probably be the first argument here. > > As you pointed out, it's common to have multiple MSIs for a single device, so we > also need a context to pass around, so my suggestion would become something like: > > struct msi_desc *msi_request(struct device *dev, irq_handler_t handler, > unsigned long flags, const char *name, void *data); > > It's possible that we have to add one or two more arguments here. Good suggestion, thanks! > >>> 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. > > Ok. > > Arnd > > . > -- Thanks! Yijing From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from szxga01-in.huawei.com ([119.145.14.64]:26136 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753452AbaHECNH (ORCPT ); Mon, 4 Aug 2014 22:13:07 -0400 Message-ID: <53E03D71.5080800@huawei.com> Date: Tue, 5 Aug 2014 10:12:01 +0800 From: Yijing Wang MIME-Version: 1.0 Subject: Re: [RFC PATCH 00/11] Refactor MSI to support Non-PCI device References: <1406344128-27055-1-git-send-email-wangyijing@huawei.com> <201408011552.32135.arnd@arndb.de> <53DF2B7B.4000605@huawei.com> <201408041659.31364.arnd@arndb.de> In-Reply-To: <201408041659.31364.arnd@arndb.de> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Arnd Bergmann Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, Russell King , Paul.Mundt@huawei.com, Marc Zyngier , linux-pci@vger.kernel.org, "James E.J. Bottomley" , virtualization@lists.linux-foundation.org, Xinwei Hu , Hanjun Guo , Bjorn Helgaas , Wuyun Message-ID: <20140805021201.TInXG5b7lmc_d3jeiuvet1KI5e2BJCZ3sBJcGG2Dlb0@z> >>> 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. > > The driver would still have the option of getting the IRQ number for now: With > the interface I imagine, you would get a 'struct msi_desc' pointer, from which > you can look up the 'struct irq_desc' pointer (either embedded in msi_desc, > or using a pointer from a member of msi_desc), and you can already get the > interrupt number from the irq_desc. > > My point was that a well-written driver already does not care about the interrupt > number: the only information a driver needs in the interrupt handler is a pointer > to its own context, which we already derive from the irq_desc. Agree, I will try to introduce this similar interface in next version, thanks! > > The main interface that currently requires the irq number is free_irq(), but > I would argue that we can just add a wrapper that takes the msi_desc pointer > as its first argument so the driver does not have to worry about it. > > We can add additional wrappers like that as needed. OK >>>> 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. > > Right, that works too. I was thinking we might need an interface that allows us to > pick a particular msi_chip if there are several alternatives (e.g. one in the GIC > and one in the PCI host), but you are right: we should normally be able to hardwire > that information in DT or elsewhere, and just need the 'struct device pointer' which > should probably be the first argument here. > > As you pointed out, it's common to have multiple MSIs for a single device, so we > also need a context to pass around, so my suggestion would become something like: > > struct msi_desc *msi_request(struct device *dev, irq_handler_t handler, > unsigned long flags, const char *name, void *data); > > It's possible that we have to add one or two more arguments here. Good suggestion, thanks! > >>> 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. > > Ok. > > Arnd > > . > -- Thanks! Yijing From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <53E03D71.5080800@huawei.com> Date: Tue, 5 Aug 2014 10:12:01 +0800 From: Yijing Wang MIME-Version: 1.0 To: Arnd Bergmann CC: , , , Russell King , , Marc Zyngier , , "James E.J. Bottomley" , , Xinwei Hu , Hanjun Guo , Bjorn Helgaas , Wuyun Subject: Re: [RFC PATCH 00/11] Refactor MSI to support Non-PCI device References: <1406344128-27055-1-git-send-email-wangyijing@huawei.com> <201408011552.32135.arnd@arndb.de> <53DF2B7B.4000605@huawei.com> <201408041659.31364.arnd@arndb.de> In-Reply-To: <201408041659.31364.arnd@arndb.de> Content-Type: text/plain; charset="ISO-8859-1" Sender: linux-arch-owner@vger.kernel.org List-ID: >>> 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. > > The driver would still have the option of getting the IRQ number for now: With > the interface I imagine, you would get a 'struct msi_desc' pointer, from which > you can look up the 'struct irq_desc' pointer (either embedded in msi_desc, > or using a pointer from a member of msi_desc), and you can already get the > interrupt number from the irq_desc. > > My point was that a well-written driver already does not care about the interrupt > number: the only information a driver needs in the interrupt handler is a pointer > to its own context, which we already derive from the irq_desc. Agree, I will try to introduce this similar interface in next version, thanks! > > The main interface that currently requires the irq number is free_irq(), but > I would argue that we can just add a wrapper that takes the msi_desc pointer > as its first argument so the driver does not have to worry about it. > > We can add additional wrappers like that as needed. OK >>>> 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. > > Right, that works too. I was thinking we might need an interface that allows us to > pick a particular msi_chip if there are several alternatives (e.g. one in the GIC > and one in the PCI host), but you are right: we should normally be able to hardwire > that information in DT or elsewhere, and just need the 'struct device pointer' which > should probably be the first argument here. > > As you pointed out, it's common to have multiple MSIs for a single device, so we > also need a context to pass around, so my suggestion would become something like: > > struct msi_desc *msi_request(struct device *dev, irq_handler_t handler, > unsigned long flags, const char *name, void *data); > > It's possible that we have to add one or two more arguments here. Good suggestion, thanks! > >>> 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. > > Ok. > > Arnd > > . > -- Thanks! Yijing From mboxrd@z Thu Jan 1 00:00:00 1970 From: wangyijing@huawei.com (Yijing Wang) Date: Tue, 5 Aug 2014 10:12:01 +0800 Subject: [RFC PATCH 00/11] Refactor MSI to support Non-PCI device In-Reply-To: <201408041659.31364.arnd@arndb.de> References: <1406344128-27055-1-git-send-email-wangyijing@huawei.com> <201408011552.32135.arnd@arndb.de> <53DF2B7B.4000605@huawei.com> <201408041659.31364.arnd@arndb.de> Message-ID: <53E03D71.5080800@huawei.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org >>> 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. > > The driver would still have the option of getting the IRQ number for now: With > the interface I imagine, you would get a 'struct msi_desc' pointer, from which > you can look up the 'struct irq_desc' pointer (either embedded in msi_desc, > or using a pointer from a member of msi_desc), and you can already get the > interrupt number from the irq_desc. > > My point was that a well-written driver already does not care about the interrupt > number: the only information a driver needs in the interrupt handler is a pointer > to its own context, which we already derive from the irq_desc. Agree, I will try to introduce this similar interface in next version, thanks! > > The main interface that currently requires the irq number is free_irq(), but > I would argue that we can just add a wrapper that takes the msi_desc pointer > as its first argument so the driver does not have to worry about it. > > We can add additional wrappers like that as needed. OK >>>> 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. > > Right, that works too. I was thinking we might need an interface that allows us to > pick a particular msi_chip if there are several alternatives (e.g. one in the GIC > and one in the PCI host), but you are right: we should normally be able to hardwire > that information in DT or elsewhere, and just need the 'struct device pointer' which > should probably be the first argument here. > > As you pointed out, it's common to have multiple MSIs for a single device, so we > also need a context to pass around, so my suggestion would become something like: > > struct msi_desc *msi_request(struct device *dev, irq_handler_t handler, > unsigned long flags, const char *name, void *data); > > It's possible that we have to add one or two more arguments here. Good suggestion, thanks! > >>> 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. > > Ok. > > Arnd > > . > -- Thanks! Yijing