| Message ID | 20180108165453.26066-9-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [01/11] powerpc/pseries: Add H_GET_CPU_CHARACTERISTICS flags & wrapper | expand |
On Tue, 9 Jan 2018, Michael Ellerman wrote: Sorry, I wasn't aware about your efforts and did not cc you. I've just queued a more generic sysfs interface for this whole mess: https://lkml.kernel.org/r/20180107214913.096657732@linutronix.de It should be simple to extend for write and it would be great if all affected architectures could share it. Thanks, tglx
Thomas Gleixner <tglx@linutronix.de> writes: > On Tue, 9 Jan 2018, Michael Ellerman wrote: > > Sorry, I wasn't aware about your efforts and did not cc you. I've just > queued a more generic sysfs interface for this whole mess: No worries. > https://lkml.kernel.org/r/20180107214913.096657732@linutronix.de > > It should be simple to extend for write and it would be great if all > affected architectures could share it. As you say this has all been a bit of a mess, and as a result we already have people running kernels with this patch, so we don't want to remove the 'rfi_flush' file. But we will certainly add support on powerpc for the files you have created, in addition to 'rfi_flush'. cheers
On 01/08/2018 05:09 PM, Michael Ellerman wrote: > Thomas Gleixner <tglx@linutronix.de> writes: > >> On Tue, 9 Jan 2018, Michael Ellerman wrote: >> >> Sorry, I wasn't aware about your efforts and did not cc you. I've just >> queued a more generic sysfs interface for this whole mess: > > No worries. > >> https://lkml.kernel.org/r/20180107214913.096657732@linutronix.de >> >> It should be simple to extend for write and it would be great if all >> affected architectures could share it. > > As you say this has all been a bit of a mess Indeed. All of us wish this went very differently. I've been testing various versions of these patches since before the holidays. For those doing backports to older kernels, a note that the IBM team added OOL (Out Of Line) exception handlers and reworked all of that code over the years since older kernels (e.g. 3.10) so you might get problems on those if you enable the debug entry. I've got notes on how to backport the OOL exceptions to older kernels if anyone cares. > and as a result we already have people running kernels with this patch, > so we don't want to remove the 'rfi_flush' file. Knowing that the IBM team was going to post with this sysfs interface, our trees contain the rfi_flush file. I mentioned it to some folks on this end (because we know we don't want to add things in sysfs generally, debugfs is a good substitute, per Andrea, and I raised this with him yesterday as a concern in the backport here) but in the end it seemed reasonable to pull this in because it was what got posted, and as Michael says, it's gone into other distro kernels beyond just ours. > But we will certainly add support on powerpc for the files you have > created, in addition to 'rfi_flush'. Thanks, Jon.
On Tue, Jan 09, 2018 at 03:54:51AM +1100, Michael Ellerman wrote: > From: Nicholas Piggin <npiggin@gmail.com> > > Expose the state of the RFI flush (enabled/disabled) via sysfs, and > allow it to be enabled/dissabled at runtime. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/kernel/setup.h | 2 ++ > arch/powerpc/kernel/sysfs.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+) You forgot a Documentation/ABI/ update for a new sysfs file :( > diff --git a/arch/powerpc/kernel/setup.h b/arch/powerpc/kernel/setup.h > index 21c18071d9d5..493b03b0a966 100644 > --- a/arch/powerpc/kernel/setup.h > +++ b/arch/powerpc/kernel/setup.h > @@ -61,4 +61,6 @@ void kvm_cma_reserve(void); > static inline void kvm_cma_reserve(void) { }; > #endif > > +extern bool rfi_flush; > + > #endif /* __ARCH_POWERPC_KERNEL_SETUP_H */ > diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c > index b8d4a1dac39f..8c19d014cffc 100644 > --- a/arch/powerpc/kernel/sysfs.c > +++ b/arch/powerpc/kernel/sysfs.c > @@ -20,6 +20,7 @@ > #include <asm/firmware.h> > > #include "cacheinfo.h" > +#include "setup.h" > > #ifdef CONFIG_PPC64 > #include <asm/paca.h> > @@ -496,6 +497,43 @@ static DEVICE_ATTR(spurr, 0400, show_spurr, NULL); > static DEVICE_ATTR(purr, 0400, show_purr, store_purr); > static DEVICE_ATTR(pir, 0400, show_pir, NULL); > > +#ifdef CONFIG_PPC_BOOK3S_64 > +static ssize_t show_rfi_flush(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", rfi_flush ? 1 : 0); > +} > + > +static ssize_t __used store_rfi_flush(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) > +{ > + int val; > + int ret = 0; > + > + ret = sscanf(buf, "%d", &val); > + if (ret != 1) > + return -EINVAL; > + > + if (val == 1) > + rfi_flush_enable(true); > + else if (val == 0) > + rfi_flush_enable(false); > + else > + return -EINVAL; > + > + return count; > +} > + > +static DEVICE_ATTR(rfi_flush, 0600, > + show_rfi_flush, store_rfi_flush); DEVICE_ATTR_RW()? And why 0600? That's odd. > + > +static void sysfs_create_rfi_flush(void) > +{ > + device_create_file(cpu_subsys.dev_root, &dev_attr_rfi_flush); No error checking? And as Thomas said, why not just use the generic infrastructure he created instead? That way there is some form of unity here for the same exact issue. At least he documented the api :) thanks, greg k-h
On Tue, Jan 09, 2018 at 01:06:23AM -0500, Jon Masters wrote: > Knowing that the IBM team was going to post with this sysfs interface, > our trees contain the rfi_flush file. I mentioned it to some folks on > this end (because we know we don't want to add things in sysfs > generally, debugfs is a good substitute, per Andrea, and I raised this > with him yesterday as a concern in the backport here) but in the end it > seemed reasonable to pull this in because it was what got posted, and as > Michael says, it's gone into other distro kernels beyond just ours. What distro kernels end up enabling does not really reflect on what we end up doing in mainline. The api for this should NOT be arch-specific if at all possible, that way lies madness. Do you want to write userspace tools to handle the 60+ different arch implementations? Don't let the fragmentation problems of the period in which no one was allowed to talk to each other, result in a unchangable mess, that would be insane. thanks, greg k-h
On 01/09/2018 03:05 AM, Greg KH wrote: > On Tue, Jan 09, 2018 at 01:06:23AM -0500, Jon Masters wrote: >> Knowing that the IBM team was going to post with this sysfs interface, >> our trees contain the rfi_flush file. I mentioned it to some folks on >> this end (because we know we don't want to add things in sysfs >> generally, debugfs is a good substitute, per Andrea, and I raised this >> with him yesterday as a concern in the backport here) but in the end it >> seemed reasonable to pull this in because it was what got posted, and as >> Michael says, it's gone into other distro kernels beyond just ours. > > What distro kernels end up enabling does not really reflect on what we > end up doing in mainline. The api for this should NOT be arch-specific > if at all possible, that way lies madness. Do you want to write > userspace tools to handle the 60+ different arch implementations? > > Don't let the fragmentation problems of the period in which no one was > allowed to talk to each other, result in a unchangable mess, that would > be insane. Totally fine :) Just saying we tried to do reasonable things with what we had. Whatever happens upstream in the end is, of course, what we'll make sure fits into updates that go into the likes of RHEL. Jon.
diff --git a/arch/powerpc/kernel/setup.h b/arch/powerpc/kernel/setup.h index 21c18071d9d5..493b03b0a966 100644 --- a/arch/powerpc/kernel/setup.h +++ b/arch/powerpc/kernel/setup.h @@ -61,4 +61,6 @@ void kvm_cma_reserve(void); static inline void kvm_cma_reserve(void) { }; #endif +extern bool rfi_flush; + #endif /* __ARCH_POWERPC_KERNEL_SETUP_H */ diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c index b8d4a1dac39f..8c19d014cffc 100644 --- a/arch/powerpc/kernel/sysfs.c +++ b/arch/powerpc/kernel/sysfs.c @@ -20,6 +20,7 @@ #include <asm/firmware.h> #include "cacheinfo.h" +#include "setup.h" #ifdef CONFIG_PPC64 #include <asm/paca.h> @@ -496,6 +497,43 @@ static DEVICE_ATTR(spurr, 0400, show_spurr, NULL); static DEVICE_ATTR(purr, 0400, show_purr, store_purr); static DEVICE_ATTR(pir, 0400, show_pir, NULL); +#ifdef CONFIG_PPC_BOOK3S_64 +static ssize_t show_rfi_flush(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", rfi_flush ? 1 : 0); +} + +static ssize_t __used store_rfi_flush(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t count) +{ + int val; + int ret = 0; + + ret = sscanf(buf, "%d", &val); + if (ret != 1) + return -EINVAL; + + if (val == 1) + rfi_flush_enable(true); + else if (val == 0) + rfi_flush_enable(false); + else + return -EINVAL; + + return count; +} + +static DEVICE_ATTR(rfi_flush, 0600, + show_rfi_flush, store_rfi_flush); + +static void sysfs_create_rfi_flush(void) +{ + device_create_file(cpu_subsys.dev_root, &dev_attr_rfi_flush); +} +#endif /* CONFIG_PPC_BOOK3S_64 */ + /* * This is the system wide DSCR register default value. Any * change to this default value through the sysfs interface @@ -1047,6 +1085,9 @@ static int __init topology_init(void) WARN_ON(r < 0); #ifdef CONFIG_PPC64 sysfs_create_dscr_default(); +#ifdef CONFIG_PPC_BOOK3S + sysfs_create_rfi_flush(); +#endif #endif /* CONFIG_PPC64 */ return 0;