Message ID | 1452395295-1759-1-git-send-email-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Rejected, archived |
Headers | show |
On Sun, 2016-01-10 at 01:08 -0200, Guilherme G. Piccoli wrote:weust changes the way the arch checking is done in function > > This patch jeeh_add_device_early(): we use no more eeh_enabled(), but instead we check therunning architecture by using the macro machine_is(). If we are running on > pSeries or PowerNV, the EEH mechanism can be enabled; otherwise, we bail out > the function. This way, we don't enable EEH on Cell and we don't hit the oops > on DLPAR either. Can't we just check for eeh_ops being NULL ? Cheers, Ben. > Fixes: 89a51df5ab1d ("powerpc/eeh: Fix crash in eeh_add_device_early() on Cell") > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- > arch/powerpc/kernel/eeh.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > index 40e4d4a..81e2d3e 100644 > --- a/arch/powerpc/kernel/eeh.c > +++ b/arch/powerpc/kernel/eeh.c > @@ -1072,7 +1072,13 @@ void eeh_add_device_early(struct pci_dn *pdn) > struct pci_controller *phb; > struct eeh_dev *edev = pdn_to_eeh_dev(pdn); > > - if (!edev || !eeh_enabled()) > + if (!edev) > + return; > + > + /* Some platforms (like Cell) don't have EEH capabilities, so we > + * need to abort here. In case of pseries or powernv, we have EEH > + * so we can continue. */ > + if (!machine_is(pseries) && !machine_is(powernv)) > return; > > if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
On Sun, 2016-01-10 at 01:08 -0200, Guilherme G. Piccoli wrote: > Commit 89a51df5ab1d ("powerpc/eeh: Fix crash in eeh_add_device_early() on Cell") > added a check on function eeh_add_device_early(): since in Cell arch eeh_ops > is NULL, that code used to crash on Cell. The commit's approach was validate > if EEH was available by checking the result of function eeh_enabled(). > > Since the function eeh_add_device_early() is used to perform EEH > initialization in devices added later on the system, like in hotplug/DLPAR > scenarios, we might reach a case in which no PCI devices are present on boot > and so EEH is not initialized. Then, if a device is added via DLPAR for > example, eeh_add_device_early() fails because eeh_enabled() is false. > > We can hit a kernel oops on pSeries arch if eeh_add_device_early() fails: > if we have no PCI devices on machine at boot time, and then we add a PCI device > via DLPAR operation, the function query_ddw() triggers the oops on NULL pointer > dereference in the line "cfg_addr = edev->config_addr;". It happens because > config_addr in edev is NULL, since the function eeh_add_device_early() was not > completed successfully. > > This patch just changes the way the arch checking is done in function > eeh_add_device_early(): we use no more eeh_enabled(), but instead we check the > running architecture by using the macro machine_is(). If we are running on > pSeries or PowerNV, the EEH mechanism can be enabled; otherwise, we bail out > the function. This way, we don't enable EEH on Cell and we don't hit the oops > on DLPAR either. But eeh_enabled() is still false? That seems like it's liable to cause breakage elsewhere. Shouldn't the PCI hotplug code instead be taught to initialise EEH correctly when the first device is added? cheers
On 01/13/2016 04:04 AM, Benjamin Herrenschmidt wrote: > On Sun, 2016-01-10 at 01:08 -0200, Guilherme G. Piccoli wrote:weust changes the way the arch checking is done in function >> >> This patch jeeh_add_device_early(): we use no more eeh_enabled(), but instead we check the running architecture by using the macro machine_is(). If we are running on >> pSeries or PowerNV, the EEH mechanism can be enabled; otherwise, we bail out >> the function. This way, we don't enable EEH on Cell and we don't hit the oops >> on DLPAR either. > > Can't we just check for eeh_ops being NULL ? > > Cheers, > Ben. Sure, we can. I prefer the arch checking just because I think it's more "logical", so it's easier to understand why it's there. The "cost" is the same in practice, since the arch checking is just a macro that checks a struct member. What do you think it's better? Thanks for the review. Cheers, Guilherme
On 01/13/2016 08:38 AM, Michael Ellerman wrote: > But eeh_enabled() is still false? That seems like it's liable to cause breakage > elsewhere. Yes, eeh_enabled() is false as expected. Notice that eeh_enabled() is telling if EEH is enabled or not, and since it's not (because there's no PCI adapters on machine yet!), makes sense it returns false. At the end of runs of eeh_add_device_early(), the devices are probed and EEH is enabled, so the function will reflect this. Notice that the problem addressed by this patch is the use of eeh_enabled() in hotplug operations only, which in my opinion is not correct. I checked every other use of eeh_enable() in the code, and they all seems appropriate, so I expect no errors at all. > Shouldn't the PCI hotplug code instead be taught to initialise EEH correctly > when the first device is added? Well, for sure there are other ways to achieve this patch's goal, like refactor PCI hotplug code in lots of places. But notice although the proposed solution is simple, it solves the issue because eeh_add_device_early() is ultimately the function used by PCI hotplug mechanism (no matter if pseries/cell/etc or the type of hotplug) to initialize EEH by probing the devices at the moment they are added to the system. In my opinion, this is exactly the location we want to change code to address this issue. What do you think? Thanks very much for the review. Cheers, Guilherme
On Wed, 2016-01-13 at 10:08 -0200, Guilherme G. Piccoli wrote: > On 01/13/2016 08:38 AM, Michael Ellerman wrote: > > But eeh_enabled() is still false? That seems like it's liable to cause breakage > > elsewhere. > > Yes, eeh_enabled() is false as expected. Notice that eeh_enabled() is > telling if EEH is enabled or not, and since it's not (because there's no > PCI adapters on machine yet!), makes sense it returns false. > > At the end of runs of eeh_add_device_early(), the devices are probed and > EEH is enabled, so the function will reflect this. Notice that the > problem addressed by this patch is the use of eeh_enabled() in hotplug > operations only, which in my opinion is not correct. I checked every > other use of eeh_enable() in the code, and they all seems appropriate, > so I expect no errors at all. OK. > > Shouldn't the PCI hotplug code instead be taught to initialise EEH correctly > > when the first device is added? > > Well, for sure there are other ways to achieve this patch's goal, like > refactor PCI hotplug code in lots of places. But notice although the > proposed solution is simple, it solves the issue because > eeh_add_device_early() is ultimately the function used by PCI hotplug > mechanism (no matter if pseries/cell/etc or the type of hotplug) to > initialize EEH by probing the devices at the moment they are added to > the system. In my opinion, this is exactly the location we want to > change code to address this issue. > > What do you think? Thanks very much for the review. I'm still not sure. I'm certainly happy for the fix to be simple, it will need to be backported after all. But for example what happens if the user boots with eeh=off on the command line, and then hotplugs a device. It looks like because you're not using eeh_enabled() you will incorrectly initialise EEH anyway? cheers
On 01/13/2016 07:25 PM, Michael Ellerman wrote: > But for example what happens if the user boots with eeh=off on the command > line, and then hotplugs a device. It looks like because you're not using > eeh_enabled() you will incorrectly initialise EEH anyway? Thanks very much for this catch Michael! I didn't think in this possibility; I just tested and it fails with the kernel oops. So, since my patch does not cover this case, I think would be more interesting "unlink" the DDW mechanism from the EEH. It seems easy, I'll try to send you a patch soon. Do you think it is a good approach? Cheers, Guilherme
On Thu, 2016-01-14 at 17:59 -0200, Guilherme G. Piccoli wrote: > On 01/13/2016 07:25 PM, Michael Ellerman wrote: > > But for example what happens if the user boots with eeh=off on the command > > line, and then hotplugs a device. It looks like because you're not using > > eeh_enabled() you will incorrectly initialise EEH anyway? > > Thanks very much for this catch Michael! I didn't think in this > possibility; I just tested and it fails with the kernel oops. OK, that's a pity. > So, since my patch does not cover this case, I think would be more > interesting "unlink" the DDW mechanism from the EEH. It seems easy, I'll > try to send you a patch soon. > > Do you think it is a good approach? It sounds good, but I don't know off hand whether it will work. See how it goes and send us the patch. cheers
On 01/14/2016 09:37 PM, Michael Ellerman wrote: >> So, since my patch does not cover this case, I think would be more >> interesting "unlink" the DDW mechanism from the EEH. It seems easy, I'll >> try to send you a patch soon. >> >> Do you think it is a good approach? > > It sounds good, but I don't know off hand whether it will work. See how it goes > and send us the patch. You are right (again!). It's kind of complicated to unlink DDW from EEH since it relies on EEH to get the config. address of devices. Instead of try to unlink it, I've inserted an EEH check to avoid issues in case DDW can't count on EEH being enabled. Will sent the patches to the list. Thanks very much for your help and comments, Guilherme
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c index 40e4d4a..81e2d3e 100644 --- a/arch/powerpc/kernel/eeh.c +++ b/arch/powerpc/kernel/eeh.c @@ -1072,7 +1072,13 @@ void eeh_add_device_early(struct pci_dn *pdn) struct pci_controller *phb; struct eeh_dev *edev = pdn_to_eeh_dev(pdn); - if (!edev || !eeh_enabled()) + if (!edev) + return; + + /* Some platforms (like Cell) don't have EEH capabilities, so we + * need to abort here. In case of pseries or powernv, we have EEH + * so we can continue. */ + if (!machine_is(pseries) && !machine_is(powernv)) return; if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
Commit 89a51df5ab1d ("powerpc/eeh: Fix crash in eeh_add_device_early() on Cell") added a check on function eeh_add_device_early(): since in Cell arch eeh_ops is NULL, that code used to crash on Cell. The commit's approach was validate if EEH was available by checking the result of function eeh_enabled(). Since the function eeh_add_device_early() is used to perform EEH initialization in devices added later on the system, like in hotplug/DLPAR scenarios, we might reach a case in which no PCI devices are present on boot and so EEH is not initialized. Then, if a device is added via DLPAR for example, eeh_add_device_early() fails because eeh_enabled() is false. We can hit a kernel oops on pSeries arch if eeh_add_device_early() fails: if we have no PCI devices on machine at boot time, and then we add a PCI device via DLPAR operation, the function query_ddw() triggers the oops on NULL pointer dereference in the line "cfg_addr = edev->config_addr;". It happens because config_addr in edev is NULL, since the function eeh_add_device_early() was not completed successfully. This patch just changes the way the arch checking is done in function eeh_add_device_early(): we use no more eeh_enabled(), but instead we check the running architecture by using the macro machine_is(). If we are running on pSeries or PowerNV, the EEH mechanism can be enabled; otherwise, we bail out the function. This way, we don't enable EEH on Cell and we don't hit the oops on DLPAR either. Fixes: 89a51df5ab1d ("powerpc/eeh: Fix crash in eeh_add_device_early() on Cell") Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> --- arch/powerpc/kernel/eeh.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)