Date:	Thu, 21 Jan 2016 14:14:50 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Michał Kępień <kernel@...pniu.pl>
Cc:	Darren Hart <dvhart@...radead.org>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Richard Purdie <rpurdie@...ys.net>,
	Jacek Anaszewski <j.anaszewski@...sung.com>,
	Alex Hung <alex.hung@...onical.com>,
	platform-driver-x86@...r.kernel.org, linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a
 separate module

On Thursday 21 January 2016 14:06:03 Michał Kępień wrote:
> > > A total of four functions have something to do with the SMBIOS buffer:
> > > 
> > >   * get_buffer()
> > >   * clear_buffer()
> > >   * release_buffer()
> > >   * dell_send_request()
> > > 
> > > This rework is a chance to make them all consistent, i.e. remove the
> > > SMBIOS buffer from their argument lists.  This way we can "signal" this
> > > API's users that there is only one SMBIOS buffer ever involved while
> > > still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
> > > also see little point in returning the buffer from dell_send_request()
> > > as none of its callers in dell-laptop assign its return value to
> > > anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
> > > the code).
> > > 
> > > To sum up, I'd suggest that function prototypes could look like this:
> > > 
> > >     struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > >     void dell_smbios_clear_buffer(void);
> > >     void dell_smbios_release_buffer(void);
> > >     void dell_smbios_send_request(int class, int select);
> > > 
> > > What do you think?
> > > 
> > 
> > In other scenario functions should do something like this:
> > 
> > struct buf *buf_alloc(void);
> > buf_clear(struct buf *buf);
> > buf_free(struct buf *buf);
> > buf_do_something(struct buf *buf, ...);
> > 
> > But here I do not know how hard is to create alloc/free functions and
> > what is cost for creating that buffer in first 4GB memory...
> 
> I'm guessing the cost is negligible, given that SMBIOS calls are not
> present in any hot path.  Writing these functions is also pretty
> straightforward, but the inconvenience of this approach is that it
> forces the callers to do the error-checking for each buf_alloc() call.
> It also seems pretty inefficient - notice we only need 36 bytes for the
> calling interface buffer, yet we would be allocating a whole page in
> each buf_alloc() call.
> 
> On the other hand, I believe returning a separate buffer for each
> buf_alloc() caller makes it possible to drop buffer_mutex altogether.
> Yet, the approach I suggested is more similar to what the Dell-supplied
> dcdbas driver does internally (it manages a single, resizable buffer,
> which is protected by a mutex and controllable from userspace through
> sysfs), which is why I think it's a good idea to stick to that concept
> for consistency.
> 
> As this patch series already touches a lot of code, I would like to
> avoid changing the underlying concepts as much as possible.  If that's
> okay with you, I'll post a v2 which includes your suggestion to make the
> buffer pointer static while keeping the interface similar to the
> original one.  If you would really like me to take a different path,
> please let me know and I'll comply.
> 

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.

But I will let decision for API to other people as I do not know what
the best API to use here...

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

Powered by blists - more mailing lists