Message-ID: <20130816223932.GB31247@kroah.com>
Date:	Fri, 16 Aug 2013 15:39:32 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Ali Ayoub <ali@....mellanox.co.il>
Cc:	Shannon Nelson <shannon.nelson@...el.com>, netdev@...r.kernel.org,
	davem@...emloft.net, dwmw2@...radead.org,
	jeffrey.t.kirsher@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] ixgbe: request_firmware for configuration parameters

On Fri, Aug 16, 2013 at 03:14:31PM -0700, Ali Ayoub wrote:
> On 1/11/2013 11:41 AM, Greg KH wrote:

Seriously?  Restarting a thread from over 6 months ago?  Why?

> > On Fri, Jan 11, 2013 at 11:30:54AM -0800, Shannon Nelson wrote:
> >> On Fri, Jan 11, 2013 at 10:25 AM, Greg KH <gregkh@...uxfoundation.org> wrote:
> >>> On Thu, Jan 10, 2013 at 06:02:20PM -0800, Shannon Nelson wrote:
> >>>> Most networking dials and knobs can be set using ethtool, ifconfig, ip link
> >>>> commands, or sysfs entries, all of which can be driven by startup scripts
> >>>> and other configuration tools.  However, they all depend on having a netdev
> >>>> already set up, and we have some low-level device functionality that needs
> >>>> to be sorted out before we start setting up MSI-x and memory allocations.
> >>
> >>> Ick, please don't abuse request_firmware() for this type of thing.
> >>
> >> Yeah, it seemed ugly to me at first as well, but it grew on me as I
> >> realized that it does solve a problem in a rather elegant way.  While
> >> working this up I discussed this with Mr. Woodhouse thinking that as a
> >> firmware tree maintainer he'd have a similar reaction, but he actually
> >> wasn't opposed to it (David, please speak up if I'm misrepresenting
> >> your comments).
> > 
> > David maintains the external firmware tree repo, not the in-kernel
> > firmware core code (which I used to maintain.)
> > 
> >>> What's wrong with configfs?  It sounds like it will fit your need, and
> >>> that is what is created for.
> >>
> >> configfs has similar problems as sysfs - the driver needs to create
> >> the hooks before it has all the info it might need for some hooks,
> >> there is no persistence across reboots, and I don't think it will help
> >> for initrd images.  Additionally, there would need to be some userland
> >> mechanism to notice that the hooks were there and to feed it the
> >> startup info.  Using a file in the firmware path gives us persistence
> >> and a way for the driver to get info before having to set up
> >> filesystem hooks.  It also gives us a way to get special config info
> >> into the boot image.  And the whole mechanism already exists,
> >> including UDEV hooks that can do more fancy stuff if needed.
> > 
> > Yes, but you are now starting to use "configuration files" for kernel
> > drivers, which we have resisted for 20+ years for a variety of good
> > reasons. You can't just ignore all of the arguments to not do this all
> > of a sudden because you feel your driver is somehow "special" here.
> 
> Other device drivers of other vendors (not only netdevs) need such a
> mechanism as well,

Specifics please.

> I think it's a general requirement for many drivers that normally need
> low level configurations for device initialization in the very first
> stage of the driver load.

I do not, but feel free to prove me otherwise.

> > All of the above issues you seem to have with sysfs and configfs can be
> > resolved with userspace code, and having your driver not do anything to
> > the hardware until it is told to by userspace.
> 
> To tell the driver not to do anything until it's configured by a
> userspace code will require a module param for non-default-configs
> (which brings us back to the original argument of avoiding module params).

No, never use a module paramater for configuring a driver or a device.
That's not what they are there for.

> By having userspace code to feed configfs/sysfs nodes, and making it
> available in initrd; we will end up having similar mechanism to
> request_firmware().

Close, but not the same.  That's why we created configfs, please use it.

> I think this kind of "low level init configuration" can be seen as a
> firmware configuration, we can put some limitation on fetching the
> config file, or propose a new function such as request_firmware_config()
> that uses the same uevent hooks, and leverages the available userspace
> tools that already supported in initrd and meant to serve the same
> purpose - of feeding the driver the suitable firmware and configuration
> to get started.

Nope, I do not, please don't abuse the interface like this, it's not
going to be allowed at this point in time.

Wait, what "point in time", this was a 6 month old conversation...

greg "what month is this?" k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists