From: Michael Ellerman <mpe@ellerman•id.au>
To: "Guilherme G. Piccoli" <gpiccoli@linux•vnet.ibm.com>
Cc: gwshan@linux•vnet.ibm.com, linuxppc-dev@lists•ozlabs.org,
paulus@samba•org,
Benjamin Herrenschmidt <benh@kernel•crashing.org>
Subject: Re: [PATCH] powerpc/eeh: Validate arch in eeh_add_device_early()
Date: Thu, 14 Jan 2016 08:25:39 +1100 [thread overview]
Message-ID: <1452720339.8203.4.camel@ellerman.id.au> (raw)
In-Reply-To: <56963E40.8070702@linux.vnet.ibm.com>
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
next prev parent reply other threads:[~2016-01-13 21:25 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-10 3:08 [PATCH] powerpc/eeh: Validate arch in eeh_add_device_early() Guilherme G. Piccoli
2016-01-13 6:04 ` Benjamin Herrenschmidt
2016-01-13 11:56 ` Guilherme G. Piccoli
2016-01-13 10:38 ` Michael Ellerman
2016-01-13 12:08 ` Guilherme G. Piccoli
2016-01-13 21:25 ` Michael Ellerman [this message]
2016-01-14 19:59 ` Guilherme G. Piccoli
2016-01-14 23:37 ` Michael Ellerman
2016-01-19 20:11 ` Guilherme G. Piccoli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1452720339.8203.4.camel@ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=benh@kernel$(echo .)crashing.org \
--cc=gpiccoli@linux$(echo .)vnet.ibm.com \
--cc=gwshan@linux$(echo .)vnet.ibm.com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=paulus@samba$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox