From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754215AbdJJTO6 (ORCPT ); Tue, 10 Oct 2017 15:14:58 -0400 Received: from esa5.dell-outbound.iphmx.com ([68.232.153.95]:62624 "EHLO esa5.dell-outbound.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751837AbdJJTOz (ORCPT ); Tue, 10 Oct 2017 15:14:55 -0400 From: X-LoopCount0: from 10.166.132.198 X-IronPort-AV: E=Sophos;i="5.43,359,1503378000"; d="scan'208";a="996202332" X-DLP: DLP_GlobalPCIDSS To: CC: , , , , , , , , , Subject: RE: [PATCH v6 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls Thread-Topic: [PATCH v6 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls Thread-Index: AQHTQfs0SZwxL5oRzk+Zgo3psGOIb6LdcwQw Date: Tue, 10 Oct 2017 19:14:53 +0000 Message-ID: <81c43f16545744b3bc0f157841fcbb1a@ausx13mpc120.AMER.DELL.COM> References: <25bf1422d8efbd631f57dcb7dd40865bae729398.1507589249.git.mario.limonciello@dell.com> <20171010160104.g4hqc7fjrp5zeg3p@pali> In-Reply-To: <20171010160104.g4hqc7fjrp5zeg3p@pali> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.143.242.75] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v9AJF2bo004696 > -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Tuesday, October 10, 2017 11:01 AM > To: Limonciello, Mario > Cc: dvhart@infradead.org; Andy Shevchenko ; > LKML ; platform-driver-x86@vger.kernel.org; > Andy Lutomirski ; quasisec@google.com; rjw@rjwysocki.net; > mjg59@google.com; hch@lst.de; Greg KH > 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@gmail.com