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

[<Mario.Limonciello@xxxxxxxx>]

> -----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];