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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Nov 2013 15:17:46 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Bjorn Helgaas <bhelgaas@...gle.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 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.

Thanks!
Neil

> Bjorn
> 
> [1] https://github.com/Irqbalance/irqbalance/blob/master/classify.c#L357
> 
--
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/