linux-kernel - Re: [RFC PATCH] PCI: export MSI mode using attributes, not kobjects

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Thu, 14 Nov 2013 14:16:04 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Veaceslav Falico <vfalico@...hat.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Yinghai Lu <yinghai@...nel.org>,
	Knut Petersen <Knut_Petersen@...nline.de>,
	Ingo Molnar <mingo@...nel.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] PCI: export MSI mode using attributes, not kobjects

On Thu, Nov 14, 2013 at 1:17 PM, Neil Horman <nhorman@...driver.com> wrote:
> On Thu, Nov 14, 2013 at 12:33:11PM -0700, Bjorn Helgaas wrote:
>> On Tue, Nov 5, 2013 at 11:55 AM, Bjorn Helgaas <bhelgaas@...gle.com> wrote:
>> > On Sat, Nov 2, 2013 at 9:50 AM, Greg Kroah-Hartman
>> > <gregkh@...uxfoundation.org> wrote:
>> >> On Fri, Nov 01, 2013 at 05:40:02PM -0600, Bjorn Helgaas wrote:
>> >>> On Tue, Oct 29, 2013 at 3:46 PM, Greg Kroah-Hartman
>> >>> <gregkh@...uxfoundation.org> wrote:
>> >>> > From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> >>> >
>> >>> > The PCI MSI sysfs code is a mess with kobjects for things that don't
>> >>> > really need to be kobjects.  This patch creates attributes dynamically
>> >>> > for the MSI interrupts instead of using kobjects.
>> >>> >
>> >>> > Note, this does not delete the existing sysfs MSI code, but puts the
>> >>> > attributes under a "msi_irqs_2" directory for testing / example.
>> >>> >
>> >>> > Also note, this removes a directory from the current MSI interrupt sysfs
>> >>> > code:
>> >>> >
>> >>> > old MSI kobjects:
>> >>> > pci_device
>> >>> >    └── msi_irqs
>> >>> >        └── 40
>> >>> >            └── mode
>> >>> >
>> >>> > new MSI attributes:
>> >>> > pci_device
>> >>> >    └── msi_irqs_2
>> >>> >        └── 40
>> >>> >
>> >>> > As there was only one file "mode" with the kobject model, the interrupt
>> >>> > number is now a file that returns the "mode" of the interrupt (msi vs.
>> >>> > msix).
>> >>> >
>> >>> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> >>> > ---
>> >>> >
>> >>> > Bjorn, I can make up a patch that rips out the existing kobject code
>> >>> > here, but I figured this patch would make things easier to follow
>> >>> > instead of having to dig through the removed logic at the same time.
>> >>> >
>> >>> > I'll clean up the error handling path for the create attribute logic as
>> >>> > well, this was just a proof of concept that this could be done.
>> >>> >
>> >>> > Do you think that anyone cares about the current mode files in sysfs to
>> >>> > move things in this manner?
>> >>>
>> >>> I like this a lot better than trying to fix all the holes in the
>> >>> current kobject code.
>> >>
>> >> Great.
>> >>
>> >>> I have no idea who, if anybody, cares about the "mode" files.  I
>> >>> assume there's a way to create the "mode" files with attributes, too?
>> >>> If so, we could replicate the existing structure with one patch, and
>> >>> simplify it with a second patch, so it would be easier to revert the
>> >>> directory change while keeping the fix.
>> >>
>> >> No, we can't create a 2-level deep attribute at the moment, only one
>> >> level, like the patch does.
>> >>
>> >> Based on Neil's comments, I think we should be fine with this as-is as
>> >> no one is messing with these files directly (which implies that we could
>> >> possibly just remove them entirely to save us the overall pain...)
>> >
>> > Hmmm.  https://bugzilla.redhat.com/show_bug.cgi?id=744012 suggests
>> > that irqbalance might be reading these files.
>>
>> I looked at the current irqbalance on github [1], and I *think* it
>> never reads the "mode" files.  It reads the entries in the "msi_irqs"
>> directory, which you're proposing to change from directories to files,
>> but I think it only uses the names.
>>
> It doesn't read the mode file (I had intended for it to, but all the information
> irqbalance needs currently is implied by the fact that it appears in the sysfs
> tree under msi_irqs in the first place).
>
>> It looks like it should be safe at least for irqbalance to make this a
>> one-level attribute.  It's possible we'll break somebody's scripts,
>> but I'm willing to try making this change because it really makes the
>> refcounting much simpler.
>>
> ACK, if you cc me on the patch that will change the sysfs directory structure,
> I'll make the corresponding changes needed to irqblanace in parallel.

I'm hoping *no* changes will be required to irqbalance.  All it seems
to care about are the names of the entries inside "msi_irqs".  The
names will stay the same; they'll just change from being directories
to being files.

If an irqbalance change *is* required, then I'm much more hesitant.  I
don't want to force people to install a new irqbalance along with a
new kernel.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/