| Message ID | 20180621181524.30550-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The function dell_smbios_smm_call and pointer platform_device are > local to the source and do not need to be in global scope, so make > them static. > > Cleans up sparse warnings: > warning: symbol 'platform_device' was not declared. Should it be static? > warning: symbol 'dell_smbios_smm_call' was not declared. Should it be > static? > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/platform/x86/dell-smbios-smm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c > index e9e9da556318..97a90bebc360 100644 > --- a/drivers/platform/x86/dell-smbios-smm.c > +++ b/drivers/platform/x86/dell-smbios-smm.c > @@ -24,7 +24,7 @@ > static int da_command_address; > static int da_command_code; > static struct calling_interface_buffer *buffer; > -struct platform_device *platform_device; > +static struct platform_device *platform_device; > static DEFINE_MUTEX(smm_mutex); > > static const struct dmi_system_id dell_device_table[] __initconst = { > @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header *dm, void *dummy) > } > } > > -int dell_smbios_smm_call(struct calling_interface_buffer *input) > +static int dell_smbios_smm_call(struct calling_interface_buffer *input) Hrm. So these are passed by pointer to dell_smbios_register_device(), which is in turn called by dell_smbios_call() from dell-smbios-base.c. So while it is valid to make these static, since we're not referencing the symbol, but the pointer value instead - I do worry about the "static" suggesting to someone reading the code that this data is not used outside of this file, when it is. I'm not finding a position on this in coding-style. Andy, do you care to weigh in on this?
On Fri, 22 Jun 2018, Darren Hart wrote: > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > The function dell_smbios_smm_call and pointer platform_device are > > local to the source and do not need to be in global scope, so make > > them static. > > > > Cleans up sparse warnings: > > warning: symbol 'platform_device' was not declared. Should it be static? > > warning: symbol 'dell_smbios_smm_call' was not declared. Should it be > > static? > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > drivers/platform/x86/dell-smbios-smm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c > > index e9e9da556318..97a90bebc360 100644 > > --- a/drivers/platform/x86/dell-smbios-smm.c > > +++ b/drivers/platform/x86/dell-smbios-smm.c > > @@ -24,7 +24,7 @@ > > static int da_command_address; > > static int da_command_code; > > static struct calling_interface_buffer *buffer; > > -struct platform_device *platform_device; > > +static struct platform_device *platform_device; > > static DEFINE_MUTEX(smm_mutex); > > > > static const struct dmi_system_id dell_device_table[] __initconst = { > > @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header *dm, void *dummy) > > } > > } > > > > -int dell_smbios_smm_call(struct calling_interface_buffer *input) > > +static int dell_smbios_smm_call(struct calling_interface_buffer *input) > > Hrm. So these are passed by pointer to dell_smbios_register_device(), which is in > turn called by dell_smbios_call() from dell-smbios-base.c. > > So while it is valid to make these static, since we're not referencing the > symbol, but the pointer value instead - I do worry about the "static" suggesting > to someone reading the code that this data is not used outside of this file, > when it is. Static protects the name. The name in this case is very generic. julia > > I'm not finding a position on this in coding-style. > > Andy, do you care to weigh in on this? > > -- > Darren Hart > VMware Open Source Technology Center > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Sat, Jun 23, 2018 at 3:22 AM, Darren Hart <dvhart@infradead.org> wrote: > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> The function dell_smbios_smm_call and pointer platform_device are >> local to the source and do not need to be in global scope, so make >> them static. >> >> Cleans up sparse warnings: >> warning: symbol 'platform_device' was not declared. Should it be static? >> warning: symbol 'dell_smbios_smm_call' was not declared. Should it be >> static? >> -int dell_smbios_smm_call(struct calling_interface_buffer *input) >> +static int dell_smbios_smm_call(struct calling_interface_buffer *input) > > Hrm. So these are passed by pointer to dell_smbios_register_device(), which is in > turn called by dell_smbios_call() from dell-smbios-base.c. > > So while it is valid to make these static, since we're not referencing the > symbol, but the pointer value instead - I do worry about the "static" suggesting > to someone reading the code that this data is not used outside of this file, > when it is. > > I'm not finding a position on this in coding-style. > > Andy, do you care to weigh in on this? We are using static keyword by almost all callback defined functions, so, for my point of view it's pretty much okay.
On Tue, Jun 26, 2018 at 10:32:17PM +0300, Andy Shevchenko wrote: > On Sat, Jun 23, 2018 at 3:22 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> The function dell_smbios_smm_call and pointer platform_device are > >> local to the source and do not need to be in global scope, so make > >> them static. > >> > >> Cleans up sparse warnings: > >> warning: symbol 'platform_device' was not declared. Should it be static? > >> warning: symbol 'dell_smbios_smm_call' was not declared. Should it be > >> static? > > > >> -int dell_smbios_smm_call(struct calling_interface_buffer *input) > >> +static int dell_smbios_smm_call(struct calling_interface_buffer *input) > > > > Hrm. So these are passed by pointer to dell_smbios_register_device(), which is in > > turn called by dell_smbios_call() from dell-smbios-base.c. > > > > So while it is valid to make these static, since we're not referencing the > > symbol, but the pointer value instead - I do worry about the "static" suggesting > > to someone reading the code that this data is not used outside of this file, > > when it is. > > > > I'm not finding a position on this in coding-style. > > > > Andy, do you care to weigh in on this? > > We are using static keyword by almost all callback defined functions, > so, for my point of view it's pretty much okay. OK, just wanted to double check.
On Sat, Jun 23, 2018 at 08:24:26AM +0200, Julia Lawall wrote: > > > On Fri, 22 Jun 2018, Darren Hart wrote: > > > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > The function dell_smbios_smm_call and pointer platform_device are > > > local to the source and do not need to be in global scope, so make > > > them static. > > > > > > Cleans up sparse warnings: > > > warning: symbol 'platform_device' was not declared. Should it be static? > > > warning: symbol 'dell_smbios_smm_call' was not declared. Should it be > > > static? > > > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > --- > > > drivers/platform/x86/dell-smbios-smm.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c > > > index e9e9da556318..97a90bebc360 100644 > > > --- a/drivers/platform/x86/dell-smbios-smm.c > > > +++ b/drivers/platform/x86/dell-smbios-smm.c > > > @@ -24,7 +24,7 @@ > > > static int da_command_address; > > > static int da_command_code; > > > static struct calling_interface_buffer *buffer; > > > -struct platform_device *platform_device; > > > +static struct platform_device *platform_device; > > > static DEFINE_MUTEX(smm_mutex); > > > > > > static const struct dmi_system_id dell_device_table[] __initconst = { > > > @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header *dm, void *dummy) > > > } > > > } > > > > > > -int dell_smbios_smm_call(struct calling_interface_buffer *input) > > > +static int dell_smbios_smm_call(struct calling_interface_buffer *input) > > > > Hrm. So these are passed by pointer to dell_smbios_register_device(), which is in > > turn called by dell_smbios_call() from dell-smbios-base.c. > > > > So while it is valid to make these static, since we're not referencing the > > symbol, but the pointer value instead - I do worry about the "static" suggesting > > to someone reading the code that this data is not used outside of this file, > > when it is. > > Static protects the name. The name in this case is very generic. Indeed "platform_device" is even more generic than I'd like for a static ;-)
diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c index e9e9da556318..97a90bebc360 100644 --- a/drivers/platform/x86/dell-smbios-smm.c +++ b/drivers/platform/x86/dell-smbios-smm.c @@ -24,7 +24,7 @@ static int da_command_address; static int da_command_code; static struct calling_interface_buffer *buffer; -struct platform_device *platform_device; +static struct platform_device *platform_device; static DEFINE_MUTEX(smm_mutex); static const struct dmi_system_id dell_device_table[] __initconst = { @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header *dm, void *dummy) } } -int dell_smbios_smm_call(struct calling_interface_buffer *input) +static int dell_smbios_smm_call(struct calling_interface_buffer *input) { struct smi_cmd command; size_t size;