RE: [PATCH v6 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls

[Mario.Limonciello]

> -----Original Message-----
> From: Pali RohÃr [mailto:pali.rohar@xxxxxxxxx]
> Sent: Tuesday, October 10, 2017 11:01 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: dvhart@xxxxxxxxxxxxx; Andy Shevchenko <andy.shevchenko@xxxxxxxxx>;
> LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx;
> Andy Lutomirski <luto@xxxxxxxxxx>; quasisec@xxxxxxxxxx; rjw@xxxxxxxxxxxxx;
> mjg59@xxxxxxxxxx; hch@xxxxxx; Greg KH <greg@xxxxxxxxx>
> Subject: Re: [PATCH v6 09/14] platform/x86: dell-smbios: Introduce dispatcher for
> SMM calls
> 
> On Monday 09 October 2017 17:51:47 Mario Limonciello wrote:
> >  static void dell_rfkill_query(struct rfkill *rfkill, void *data)
> >  {
> > -	struct calling_interface_buffer *buffer;
> >  	int radio = ((unsigned long)data & 0xF);
> >  	int hwswitch;
> >  	int status;
> >  	int ret;
> >
> > -	buffer = dell_smbios_get_buffer();
> > -
> > -	dell_smbios_send_request(17, 11);
> > -	ret = buffer->output[0];
> > +	ret = dell_send_request(17, 11, 0, 0, 0, 0);
> 
> Basically I do not like function which takes ten numeric parameters.
> Before in this code there was just function with 2 parameters, now there
> are lot of zero parameters, without information what which parameter
> means...
> 

In an earlier patch Darren wanted to keep dell_send_request around and
remove code that was duplicated multiple times to clear buffer, set arguments etc,
can you please comment on what exactly you would prefer?

It's very obvious if you look at the dell_send_request function what is happening.

> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-
> smbios.c
> > index ade248646578..386130811a34 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > +void dell_smbios_get_smm_address(int *address, int *code)
> > +{
> > +	*address = da_command_address;
> > +	*code = da_command_code;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_smbios_get_smm_address);
> 
> Should not be this function in dell-smbios-smm driver instead of the
> dispatcher? It has in name "smm" and also is specific for smm driver...

That would mean parsing DA table in both drivers.  Since it was already
available in dell-smbios driver, it made sense to me to export it to the other
driver this way.

> 
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 90d7e8e55e9b..b74f48d17116 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -641,13 +641,16 @@ static int dell_wmi_events_set_enabled(bool enable)
> >  	struct calling_interface_buffer *buffer;
> >  	int ret;
> >
> > -	buffer = dell_smbios_get_buffer();
> > +	buffer = (void *)get_zeroed_page(GFP_KERNEL);
> 
> It is still needed to ask for page? Is not enough to allocate memory via
> some *alloc function?

I think this particular one may be able to get away with less memory than a page.
I'll confirm.

> 
> > +	buffer->class = 17;
> > +	buffer->select = 3;
> >  	buffer->input[0] = 0x10000;
> >  	buffer->input[1] = 0x51534554;
> >  	buffer->input[3] = enable;
> > -	dell_smbios_send_request(17, 3);
> > -	ret = buffer->output[0];
> > -	dell_smbios_release_buffer();
> > +	ret = dell_smbios_call(buffer);
> > +	if (ret == 0)
> > +		ret = buffer->output[0];
> > +	free_page((unsigned long)buffer);
> >
> >  	return dell_smbios_error(ret);
> >  }
> 
> --
> Pali RohÃr
> pali.rohar@xxxxxxxxx