RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

From: Mario.Limonciello
Date: Wed Jan 31 2018 - 11:47:25 EST


> -----Original Message-----
> From: Pali RohÃr [mailto:pali.rohar@xxxxxxxxx]
> Sent: Wednesday, January 31, 2018 3:08 AM
> To: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Cc: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>; Darren Hart
> <dvhart@xxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Platform Driver
> <platform-driver-x86@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
>
> On Tuesday 30 January 2018 20:14:26 Andy Shevchenko wrote:
> > >> -static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >> +static void dell_set_arguments(struct calling_interface_buffer *buffer,
> > >> + u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >
> > > Hm... this function has too many parameters :-(
> > >
> > > What about following API?
> > >
> > > static struct calling_interface_buffer dell_set_arguments(u32 arg0, u32 arg1,
> u32 arg2, u32 arg3);
> > >
> > > It would return filled structure (not a pointer !)
> >
> > I do not think it's a good idea. Either we allocate request on a heap
> > and return a pointer, or we fill the struct with some data on spot.
> >
> > To naming:
> >
> > for allocation: ..._alloc_request()
> > for filling: _fill_request() / _prepare_request()
> >
> > or alike.
> >
> > _set_arguments() not good enough to me.
>
> Ok. Then we need to stick with 5 arguments... What about name
> dell_fill_request()? E.g.
>
> struct calling_interface_buffer buffer;
> dell_fill_request(&buffer, 0x2, 0, 0, 0);
> ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
>

The other alternative is to just define the input of the structure immediately with
an initializer, no multi argument filler function. Like this:

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index ab58373..e7a971b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -469,13 +469,15 @@ static int dell_rfkill_set(void *data, bool blocked)
int disable = blocked ? 1 : 0;
unsigned long radio = (unsigned long)data;
int hwswitch_bit = (unsigned long)data - 1;
- struct calling_interface_buffer buffer;
+ struct calling_interface_buffer buffer = {CLASS_INFO,
+ SELECT_RFKILL,
+ {0, 0, 0, 0},
+ {0, 0, 0, 0}};
int hwswitch;
int status;
int ret;

- dell_set_arguments(&buffer, 0, 0, 0, 0);
- ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+ ret = dell_send_request(&buffer);
if (ret)
return ret;
status = buffer.output[1];