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@xxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
