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

Groups > linux.kernel > #1329604 > unrolled thread

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

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 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Darren Hart <dvhart@infradead.org> - 2016-02-08 22:50 +0100
    Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Pali Rohár <pali.rohar@gmail.com> - 2016-02-09 09:40 +0100
      Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-02-09 15:00 +0100
      Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Darren Hart <dvhart@infradead.org> - 2016-02-09 18:00 +0100
        Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module Pali Rohár <pali.rohar@gmail.com> - 2016-02-09 20:20 +0100
          Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Darren Hart <dvhart@infradead.org> - 2016-02-11 00:10 +0100

#1329604 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

On Thu, Jan 21, 2016 at 02:39:21PM +0100, Michał Kępień wrote:
> > Another idea:
> > 
> > What about passing struct calling_interface_buffer from caller allocated
> > memory (either from stack or kernel alloc) to dell-smbios which will
> > copy it into own buffer under 4GB and then pass it to dcdbas?
> > 
> > This will avoid to use that get/release function and there will be only
> > one send_request.
> 
> Well, yes, these two functions could then be ripped out, but the callers
> would have to do the error checking on their own.  Of course that's not
> a bad thing per se, but it changes the currently used concept.
> 
> > But I will let decision for API to other people as I do not know what
> > the best API to use here...
> 
> In order to avoid delaying this any further, I'll post a v2 soon and
> hopefully it'll be good enough for your Acked-by.  If it turns out more
> people have misgivings about it, I'll adjust the code.
> 

It seems to me the question is primarily over whether or not the interface
protects a shared resource (a common buffer) or if that buffer should be
independent for every caller.

I favor minimal and incremental change and avoiding complexity where it is not
needed. I don't believe there is anything performance critical in any of these
paths, e.g. nothing requires a better response time than about 100ms and nothing
is likely to occur at a frequency above 10Hz or so.

Assuming the above is an accurate view, I don't see any reason to go beyond the
minimal change to the existing SMBIOS code to make it a usable API. If the need
arises, we can always make such optimizations and performance improvements
later. This is an internal API and we can change it whenever we need to so long
as we update the call sites.

Does anyone have a compelling reason to look for changes to the v2 submitted by
Michał?

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [next] | [standalone]

#1329964

On Monday 08 February 2016 13:42:10 Darren Hart wrote:
> Assuming the above is an accurate view, I don't see any reason to go beyond the
> minimal change to the existing SMBIOS code to make it a usable API. If the need
> arises, we can always make such optimizations and performance improvements
> later. This is an internal API and we can change it whenever we need to so long
> as we update the call sites.

Problem is that now smbios code from dell-laptop.c is moved into
dell-smbios.c and dell-smbios.h and LED subsystem starts using
dell-smbios.h. In this case I'm thinking that we have something like API
usable by other modules/subsystem. And I'm thinking if it is not better
to create "correct" API now instead rewriting code in LED and platform
subsystem again later... As this API needs to provide just 1 function,
send command to Dell SMBIOS I think that API is still minimal. Currently
we have another two functions alloc/free buffer (needed for send).

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

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

#1330247

> > Assuming the above is an accurate view, I don't see any reason to go beyond the
> > minimal change to the existing SMBIOS code to make it a usable API. If the need
> > arises, we can always make such optimizations and performance improvements
> > later. This is an internal API and we can change it whenever we need to so long
> > as we update the call sites.
> 
> Problem is that now smbios code from dell-laptop.c is moved into
> dell-smbios.c and dell-smbios.h and LED subsystem starts using
> dell-smbios.h. In this case I'm thinking that we have something like API
> usable by other modules/subsystem. And I'm thinking if it is not better
> to create "correct" API now instead rewriting code in LED and platform
> subsystem again later... As this API needs to provide just 1 function,
> send command to Dell SMBIOS I think that API is still minimal. Currently
> we have another two functions alloc/free buffer (needed for send).

Pali, forgive me, but I fail to understand your last two sentences.
Currently, we don't have functions for allocating/freeing the SMBIOS
buffer as it is allocated in dell_init() (or dell_smbios_init() after
applying this patch series) and freed in dell_(smbios_)exit().

In your previous messages [1][2], you suggested that we could allocate a
separate SMBIOS buffer for each caller or copy input parameters from
caller-allocated memory into a module-wide SMBIOS buffer before
performing the SMBIOS request.  Did you just mean to remind Darren of
these ideas or did you have something else on your mind?  If the latter,
could you please try rephrasing the last two sentences of your message?

[1] http://www.spinics.net/lists/platform-driver-x86/msg08260.html
[2] http://www.spinics.net/lists/platform-driver-x86/msg08268.html

-- 
Best regards,
Michał Kępień

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

#1330470

On Tue, Feb 09, 2016 at 09:33:03AM +0100, Pali Rohár wrote:
> On Monday 08 February 2016 13:42:10 Darren Hart wrote:
> > Assuming the above is an accurate view, I don't see any reason to go beyond the
> > minimal change to the existing SMBIOS code to make it a usable API. If the need
> > arises, we can always make such optimizations and performance improvements
> > later. This is an internal API and we can change it whenever we need to so long
> > as we update the call sites.
> 
> Problem is that now smbios code from dell-laptop.c is moved into
> dell-smbios.c and dell-smbios.h and LED subsystem starts using
> dell-smbios.h. In this case I'm thinking that we have something like API
> usable by other modules/subsystem. And I'm thinking if it is not better
> to create "correct" API now instead rewriting code in LED and platform
> subsystem again later... As this API needs to provide just 1 function,
> send command to Dell SMBIOS I think that API is still minimal. Currently
> we have another two functions alloc/free buffer (needed for send).

The internal kernel API changes all the time, we are not bound to it beyond
ensuring we update the internal users when we change it. I prefer not to
introduce complexity until we have to.

	buffer = dell_smbios_get_buffer();
	buffer->input[0] = token->location;
	buffer->input[1] = token->value;
	dell_smbios_send_request(1, 0);
	dell_smbios_release_buffer();

The get_buffer and release_buffer also include the locking which is necessary
for a shared buffer. If you eliminate the shared buffer, then you have to have a
local buffer, which adds back code to create the buffer, initializize
it, free it if it's dynamic, etc.

So from that sense, Michał's API seems at least as concise as the alternative,
and it introduces less change.

-- 
Darren Hart
Intel Open Source Technology Center

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

#1330599 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

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

On Tuesday 09 February 2016 17:51:06 Darren Hart wrote:
> On Tue, Feb 09, 2016 at 09:33:03AM +0100, Pali Rohár wrote:
> > On Monday 08 February 2016 13:42:10 Darren Hart wrote:
> > > Assuming the above is an accurate view, I don't see any reason to
> > > go beyond the minimal change to the existing SMBIOS code to make
> > > it a usable API. If the need arises, we can always make such
> > > optimizations and performance improvements later. This is an
> > > internal API and we can change it whenever we need to so long as
> > > we update the call sites.
> > 
> > Problem is that now smbios code from dell-laptop.c is moved into
> > dell-smbios.c and dell-smbios.h and LED subsystem starts using
> > dell-smbios.h. In this case I'm thinking that we have something
> > like API usable by other modules/subsystem. And I'm thinking if it
> > is not better to create "correct" API now instead rewriting code
> > in LED and platform subsystem again later... As this API needs to
> > provide just 1 function, send command to Dell SMBIOS I think that
> > API is still minimal. Currently we have another two functions
> > alloc/free buffer (needed for send).
> 
> The internal kernel API changes all the time, we are not bound to it
> beyond ensuring we update the internal users when we change it. I
> prefer not to introduce complexity until we have to.
> 
> 	buffer = dell_smbios_get_buffer();
> 	buffer->input[0] = token->location;
> 	buffer->input[1] = token->value;
> 	dell_smbios_send_request(1, 0);
> 	dell_smbios_release_buffer();
> 
> The get_buffer and release_buffer also include the locking which is
> necessary for a shared buffer. If you eliminate the shared buffer,
> then you have to have a local buffer, which adds back code to create
> the buffer, initializize it, free it if it's dynamic, etc.
> 
> So from that sense, Michał's API seems at least as concise as the
> alternative, and it introduces less change.

Ok. Then let's stay with it. I have not tested patches yet, but do not 
see anything wrong. So go ahead you can add my Reviewed-by.

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

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

#1331564

On Tue, Feb 09, 2016 at 08:12:07PM +0100, Pali Rohár wrote:
> On Tuesday 09 February 2016 17:51:06 Darren Hart wrote:
> > On Tue, Feb 09, 2016 at 09:33:03AM +0100, Pali Rohár wrote:
> > > On Monday 08 February 2016 13:42:10 Darren Hart wrote:
> > > > Assuming the above is an accurate view, I don't see any reason to
> > > > go beyond the minimal change to the existing SMBIOS code to make
> > > > it a usable API. If the need arises, we can always make such
> > > > optimizations and performance improvements later. This is an
> > > > internal API and we can change it whenever we need to so long as
> > > > we update the call sites.
> > > 
> > > Problem is that now smbios code from dell-laptop.c is moved into
> > > dell-smbios.c and dell-smbios.h and LED subsystem starts using
> > > dell-smbios.h. In this case I'm thinking that we have something
> > > like API usable by other modules/subsystem. And I'm thinking if it
> > > is not better to create "correct" API now instead rewriting code
> > > in LED and platform subsystem again later... As this API needs to
> > > provide just 1 function, send command to Dell SMBIOS I think that
> > > API is still minimal. Currently we have another two functions
> > > alloc/free buffer (needed for send).
> > 
> > The internal kernel API changes all the time, we are not bound to it
> > beyond ensuring we update the internal users when we change it. I
> > prefer not to introduce complexity until we have to.
> > 
> > 	buffer = dell_smbios_get_buffer();
> > 	buffer->input[0] = token->location;
> > 	buffer->input[1] = token->value;
> > 	dell_smbios_send_request(1, 0);
> > 	dell_smbios_release_buffer();
> > 
> > The get_buffer and release_buffer also include the locking which is
> > necessary for a shared buffer. If you eliminate the shared buffer,
> > then you have to have a local buffer, which adds back code to create
> > the buffer, initializize it, free it if it's dynamic, etc.
> > 
> > So from that sense, Michał's API seems at least as concise as the
> > alternative, and it introduces less change.
> 
> Ok. Then let's stay with it. I have not tested patches yet, but do not 
> see anything wrong. So go ahead you can add my Reviewed-by.

Great. Thank you Pali.

This has been queued to testing.

Thank you all for your diligence in getting this series to this point.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web