Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1329498 > unrolled thread

Re: [PATCH v2 00/16] Common Dell SMBIOS API

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 00/16] Common Dell SMBIOS API Darren Hart <dvhart@infradead.org> - 2016-02-08 20:30 +0100
    Re: [PATCH v2 00/16] Common Dell SMBIOS API Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-08 20:30 +0100
      Re: [PATCH v2 00/16] Common Dell SMBIOS API Darren Hart <dvhart@infradead.org> - 2016-02-08 21:50 +0100
        Re: [PATCH v2 00/16] Common Dell SMBIOS API Pali Rohár <pali.rohar@gmail.com> - 2016-02-08 22:10 +0100
          Re: [PATCH v2 00/16] Common Dell SMBIOS API Darren Hart <dvhart@infradead.org> - 2016-02-08 22:40 +0100
        Re: [PATCH v2 00/16] Common Dell SMBIOS API Michał Kępień <kernel@kempniu.pl> - 2016-02-09 15:20 +0100

#1329498 — Re: [PATCH v2 00/16] Common Dell SMBIOS API

On Fri, Jan 22, 2016 at 03:48:51PM +0100, Pali Rohár wrote:
> On Friday 22 January 2016 15:27:12 Michał Kępień wrote:
> > Note:
> > 
> >     In this series (both v1 and v2) I tried to stick to the overall
> >     concept used in dell-laptop, but in the v1 thread me and Pali also
> >     briefly discussed his alternative ideas [1][2] as to what this API
> >     could look like, so feel free to suggest a different approach.
> > 
> > [1] http://www.spinics.net/lists/platform-driver-x86/msg08260.html
> > [2] http://www.spinics.net/lists/platform-driver-x86/msg08268.html
> 
> I would like to hear opinion about dell-smbios API also from other
> people. Darren, can you look and comment it?

This is an excellently prepared series, nice work Michał.

Most of my concerns were addressed by later patches in the series. I have pushed
a version of this 1/7 fixed per lkp (linux/io.h) and 7/16 with a corrected body
as I sent in reply to that patch. This is on my tree as the dell-smbios branch.

My only major concern is module load order dependencies. Inter-module
dependencies are frowned upon with good reason, the kernel load ordering is
non-deterministic and it's possible, for example, for dell-laptop to fail to
find the symbols exported by dell-smbios under certain conditions.

I have worked around this in the past with things like the following:

#ifdef MODULE
#ifdef CONFIG_FOO_MODULE
	if (request_module("foo"))
		return -ENODEV;
#endif
#endif

Something like the above may be necessary for dell-smbios in dell-laptop,
dell-wmi, and dell-leds now that they depend on the dell-smbios exported
functions.

Cc Greg in case there is a better way to handle this that I'm not aware of.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [next] | [standalone]

#1329500

On Mon, Feb 08, 2016 at 11:20:14AM -0800, Darren Hart wrote:
> On Fri, Jan 22, 2016 at 03:48:51PM +0100, Pali Rohár wrote:
> > On Friday 22 January 2016 15:27:12 Michał Kępień wrote:
> > > Note:
> > > 
> > >     In this series (both v1 and v2) I tried to stick to the overall
> > >     concept used in dell-laptop, but in the v1 thread me and Pali also
> > >     briefly discussed his alternative ideas [1][2] as to what this API
> > >     could look like, so feel free to suggest a different approach.
> > > 
> > > [1] http://www.spinics.net/lists/platform-driver-x86/msg08260.html
> > > [2] http://www.spinics.net/lists/platform-driver-x86/msg08268.html
> > 
> > I would like to hear opinion about dell-smbios API also from other
> > people. Darren, can you look and comment it?
> 
> This is an excellently prepared series, nice work Michał.
> 
> Most of my concerns were addressed by later patches in the series. I have pushed
> a version of this 1/7 fixed per lkp (linux/io.h) and 7/16 with a corrected body
> as I sent in reply to that patch. This is on my tree as the dell-smbios branch.
> 
> My only major concern is module load order dependencies. Inter-module
> dependencies are frowned upon with good reason, the kernel load ordering is
> non-deterministic and it's possible, for example, for dell-laptop to fail to
> find the symbols exported by dell-smbios under certain conditions.
> 
> I have worked around this in the past with things like the following:
> 
> #ifdef MODULE
> #ifdef CONFIG_FOO_MODULE
> 	if (request_module("foo"))
> 		return -ENODEV;
> #endif
> #endif
> 
> Something like the above may be necessary for dell-smbios in dell-laptop,
> dell-wmi, and dell-leds now that they depend on the dell-smbios exported
> functions.
> 
> Cc Greg in case there is a better way to handle this that I'm not aware of.

No need to request_module anything, the symbol resolution should pull
the dependant module in automagically when you do a 'modprobe',
otherwise the symbols would never be found.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1329554

On Mon, Feb 08, 2016 at 11:29:20AM -0800, Greg Kroah-Hartman wrote:
> On Mon, Feb 08, 2016 at 11:20:14AM -0800, Darren Hart wrote:
> > On Fri, Jan 22, 2016 at 03:48:51PM +0100, Pali Rohár wrote:
> > > On Friday 22 January 2016 15:27:12 Michał Kępień wrote:
> > > > Note:
> > > > 
> > > >     In this series (both v1 and v2) I tried to stick to the overall
> > > >     concept used in dell-laptop, but in the v1 thread me and Pali also
> > > >     briefly discussed his alternative ideas [1][2] as to what this API
> > > >     could look like, so feel free to suggest a different approach.
> > > > 
> > > > [1] http://www.spinics.net/lists/platform-driver-x86/msg08260.html
> > > > [2] http://www.spinics.net/lists/platform-driver-x86/msg08268.html
> > > 
> > > I would like to hear opinion about dell-smbios API also from other
> > > people. Darren, can you look and comment it?
> > 
> > This is an excellently prepared series, nice work Michał.
> > 
> > Most of my concerns were addressed by later patches in the series. I have pushed
> > a version of this 1/7 fixed per lkp (linux/io.h) and 7/16 with a corrected body
> > as I sent in reply to that patch. This is on my tree as the dell-smbios branch.
> > 
> > My only major concern is module load order dependencies. Inter-module
> > dependencies are frowned upon with good reason, the kernel load ordering is
> > non-deterministic and it's possible, for example, for dell-laptop to fail to
> > find the symbols exported by dell-smbios under certain conditions.
> > 
> > I have worked around this in the past with things like the following:
> > 
> > #ifdef MODULE
> > #ifdef CONFIG_FOO_MODULE
> > 	if (request_module("foo"))
> > 		return -ENODEV;
> > #endif
> > #endif
> > 
> > Something like the above may be necessary for dell-smbios in dell-laptop,
> > dell-wmi, and dell-leds now that they depend on the dell-smbios exported
> > functions.
> > 
> > Cc Greg in case there is a better way to handle this that I'm not aware of.
> 
> No need to request_module anything, the symbol resolution should pull
> the dependant module in automagically when you do a 'modprobe',
> otherwise the symbols would never be found.

OK, Thank you Greg. Based on this, I have no concerns that haven't been addressed in the
pdx86/dell-smbios branch.

Michał, did you have any changes you wanted to make?

Pali, are you happy enough with this to add your reviewed-by?

I'm satisfied with the series and can push to next tomorrow if we're all in
agreement.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1329572

[Multipart message — attachments visible in raw view] — view raw

On Monday 08 February 2016 21:46:46 Darren Hart wrote:
> Pali, are you happy enough with this to add your reviewed-by?

There was dicussion about dell-smbios API which you probably missed in
tons of other emails. It has subject:

"[PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module"

And you can find it in archive at:
http://www.spinics.net/lists/linux-leds/msg05379.html

I wanted to hear your opinion about this API and I'm not 100% fine with
it but on other side it is not easy to design better... Maybe you could
have better idea.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1329592

On Mon, Feb 08, 2016 at 10:04:32PM +0100, Pali Rohár wrote:
> On Monday 08 February 2016 21:46:46 Darren Hart wrote:
> > Pali, are you happy enough with this to add your reviewed-by?
> 
> There was dicussion about dell-smbios API which you probably missed in
> tons of other emails. It has subject:
> 
> "[PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module"
> 
> And you can find it in archive at:
> http://www.spinics.net/lists/linux-leds/msg05379.html
> 
> I wanted to hear your opinion about this API and I'm not 100% fine with
> it but on other side it is not easy to design better... Maybe you could
> have better idea.

I will review the above. However, a new API can be treated as separate from
the refactoring which is all this series really does. It doesn't do anything
that I saw beyond moving existing code into a separate module and wrapping the
use of the buffer and tokens. In that sense, it seems to me that this can be
considered a first step toward a redesigned API by first removing duplicate code
and reusing some of the existing code.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1330273

> > > Most of my concerns were addressed by later patches in the series. I have pushed
> > > a version of this 1/7 fixed per lkp (linux/io.h) and 7/16 with a corrected body
> > > as I sent in reply to that patch. This is on my tree as the dell-smbios branch.

Thanks!

> > > 
> > > My only major concern is module load order dependencies. Inter-module
> > > dependencies are frowned upon with good reason, the kernel load ordering is
> > > non-deterministic and it's possible, for example, for dell-laptop to fail to
> > > find the symbols exported by dell-smbios under certain conditions.
> > > 
> > > I have worked around this in the past with things like the following:
> > > 
> > > #ifdef MODULE
> > > #ifdef CONFIG_FOO_MODULE
> > > 	if (request_module("foo"))
> > > 		return -ENODEV;
> > > #endif
> > > #endif
> > > 
> > > Something like the above may be necessary for dell-smbios in dell-laptop,
> > > dell-wmi, and dell-leds now that they depend on the dell-smbios exported
> > > functions.
> > > 
> > > Cc Greg in case there is a better way to handle this that I'm not aware of.
> > 
> > No need to request_module anything, the symbol resolution should pull
> > the dependant module in automagically when you do a 'modprobe',
> > otherwise the symbols would never be found.
> 
> OK, Thank you Greg. Based on this, I have no concerns that haven't been addressed in the
> pdx86/dell-smbios branch.
> 
> Michał, did you have any changes you wanted to make?

No, thanks, nothing new popped up.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web