diff mbox

x86/platform/intel-mid: Constify mid_pci_platform_pm

Message ID c2b767a782f9a0e1780e5c201d245d1be5b7d78a.1476011255.git.lukas@wunner.de (mailing list archive)
State New, archived
Headers show

Commit Message

Lukas Wunner Oct. 9, 2016, 11:12 a.m. UTC
-.data          56
+.data           0
-.rodata        32
+.rodata        88

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci-mid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andy Shevchenko Oct. 9, 2016, 2:01 p.m. UTC | #1
On Sun, 2016-10-09 at 13:12 +0200, Lukas Wunner wrote:
> -.data          56
> +.data           0
> -.rodata        32
> +.rodata        88
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---
>  drivers/pci/pci-mid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
> index a8b52dc..566ded1 100644
> --- a/drivers/pci/pci-mid.c
> +++ b/drivers/pci/pci-mid.c
> @@ -54,7 +54,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
>  	return false;
>  }
>  
> -static struct pci_platform_pm_ops mid_pci_platform_pm = {
> +static const struct pci_platform_pm_ops mid_pci_platform_pm = {
>  	.is_manageable	= mid_pci_power_manageable,
>  	.set_state	= mid_pci_set_power_state,
>  	.get_state	= mid_pci_get_power_state,
Lukas Wunner Nov. 28, 2016, 11:29 a.m. UTC | #2
On Sun, Oct 09, 2016 at 05:01:18PM +0300, Andy Shevchenko wrote:
> On Sun, 2016-10-09 at 13:12 +0200, Lukas Wunner wrote:
> > -.data          56
> > +.data           0
> > -.rodata        32
> > +.rodata        88
> > 
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Hi Bjorn,

it seems this patch wasn't applied to one of your branches.
Would you prefer this to go in via tip.git, like we did
with the other two intel-mid patches I submitted?  If so,
could you ack this patch?  I'd then resend with your ack
and cc: the x86 maintainers.

Thanks,

Lukas

> 
> > ---
> >  drivers/pci/pci-mid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
> > index a8b52dc..566ded1 100644
> > --- a/drivers/pci/pci-mid.c
> > +++ b/drivers/pci/pci-mid.c
> > @@ -54,7 +54,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
> >  	return false;
> >  }
> >  
> > -static struct pci_platform_pm_ops mid_pci_platform_pm = {
> > +static const struct pci_platform_pm_ops mid_pci_platform_pm = {
> >  	.is_manageable	= mid_pci_power_manageable,
> >  	.set_state	= mid_pci_set_power_state,
> >  	.get_state	= mid_pci_get_power_state,
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Nov. 28, 2016, 7:25 p.m. UTC | #3
On Sun, Oct 09, 2016 at 01:12:55PM +0200, Lukas Wunner wrote:
> -.data          56
> +.data           0
> -.rodata        32
> +.rodata        88
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

It'd be nice if this had a changelog.  I'm happy if this goes via the
x86 tree.

I can't remember a discussion about having this code in drivers/pci in
the first place.  Would it make sense to move it to
arch/x86/platform/intel-mid/?

8e522e1d321b ("x86/platform/intel-mid: Add Intel Penwell to ID table")
fixed a sync issue and added a comment about staying in sync with
arch/x86/platform/intel-mid/pwr.c.  Maybe moving this code to arch/x86
would help with that?

Looks like we'd have to expose pci_platform_pm_ops and
pci_set_platform_pm(), but setting platform-specific PM ops does seem
like something that would fit in the arch directories, so maybe that
wouldn't be a bad thing.

> ---
>  drivers/pci/pci-mid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
> index a8b52dc..566ded1 100644
> --- a/drivers/pci/pci-mid.c
> +++ b/drivers/pci/pci-mid.c
> @@ -54,7 +54,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
>  	return false;
>  }
>  
> -static struct pci_platform_pm_ops mid_pci_platform_pm = {
> +static const struct pci_platform_pm_ops mid_pci_platform_pm = {
>  	.is_manageable	= mid_pci_power_manageable,
>  	.set_state	= mid_pci_set_power_state,
>  	.get_state	= mid_pci_get_power_state,
> -- 
> 2.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Nov. 28, 2016, 7:43 p.m. UTC | #4
On Mon, 2016-11-28 at 13:25 -0600, Bjorn Helgaas wrote:

+Cc: Rafael

> I can't remember a discussion about having this code in drivers/pci in
> the first place.  Would it make sense to move it to
> arch/x86/platform/intel-mid/?
> 
> 8e522e1d321b ("x86/platform/intel-mid: Add Intel Penwell to ID table")
> fixed a sync issue and added a comment about staying in sync with
> arch/x86/platform/intel-mid/pwr.c.  Maybe moving this code to arch/x86
> would help with that?
> 
> Looks like we'd have to expose pci_platform_pm_ops and
> pci_set_platform_pm(), but setting platform-specific PM ops does seem
> like something that would fit in the arch directories, so maybe that
> wouldn't be a bad thing.

We have pci-acpi.c there which is used AFAIU by drivers/acpi. I'm not
sure that it's a good idea to spread users of pci_platform_pm_ops under
arch/x86 and drivers/acpi. OTOH I have no strong opinion. Whatever fits
better.
Lukas Wunner Nov. 28, 2016, 10:38 p.m. UTC | #5
On Mon, Nov 28, 2016 at 01:25:30PM -0600, Bjorn Helgaas wrote:
> On Sun, Oct 09, 2016 at 01:12:55PM +0200, Lukas Wunner wrote:
> > -.data          56
> > +.data           0
> > -.rodata        32
> > +.rodata        88
> > 
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> It'd be nice if this had a changelog.  I'm happy if this goes via the
> x86 tree.

Thanks, I'll resend with your ack.

As for the changelog, to be honest I can't think of much more to write
there.  One important motivation of constifying structs that are never
modified, and particularly structs containing function pointers like
this one, is to prevent their modification and subsequent usage by an
attacker.  However constification patches are submitted all the
time by Julia Lawall and others, and I've never seen this rationale
spelled out in a commit message, so the assumption seems to be that
it's common knowledge.  I could probably add something like

	Size of pci-mid.o ELF sections:

to clarify what the numbers in the changelog refer to.

Best regards,

Lukas

> I can't remember a discussion about having this code in drivers/pci in
> the first place.  Would it make sense to move it to
> arch/x86/platform/intel-mid/?
> 
> 8e522e1d321b ("x86/platform/intel-mid: Add Intel Penwell to ID table")
> fixed a sync issue and added a comment about staying in sync with
> arch/x86/platform/intel-mid/pwr.c.  Maybe moving this code to arch/x86
> would help with that?
> 
> Looks like we'd have to expose pci_platform_pm_ops and
> pci_set_platform_pm(), but setting platform-specific PM ops does seem
> like something that would fit in the arch directories, so maybe that
> wouldn't be a bad thing.
> 
> > ---
> >  drivers/pci/pci-mid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
> > index a8b52dc..566ded1 100644
> > --- a/drivers/pci/pci-mid.c
> > +++ b/drivers/pci/pci-mid.c
> > @@ -54,7 +54,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
> >  	return false;
> >  }
> >  
> > -static struct pci_platform_pm_ops mid_pci_platform_pm = {
> > +static const struct pci_platform_pm_ops mid_pci_platform_pm = {
> >  	.is_manageable	= mid_pci_power_manageable,
> >  	.set_state	= mid_pci_set_power_state,
> >  	.get_state	= mid_pci_get_power_state,
> > -- 
> > 2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Nov. 29, 2016, 4:40 a.m. UTC | #6
On Mon, Nov 28, 2016 at 11:38:59PM +0100, Lukas Wunner wrote:
> On Mon, Nov 28, 2016 at 01:25:30PM -0600, Bjorn Helgaas wrote:
> > On Sun, Oct 09, 2016 at 01:12:55PM +0200, Lukas Wunner wrote:
> > > -.data          56
> > > +.data           0
> > > -.rodata        32
> > > +.rodata        88
> > > 
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > 
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > It'd be nice if this had a changelog.  I'm happy if this goes via the
> > x86 tree.
> 
> Thanks, I'll resend with your ack.
> 
> As for the changelog, to be honest I can't think of much more to write
> there.  One important motivation of constifying structs that are never
> modified, and particularly structs containing function pointers like
> this one, is to prevent their modification and subsequent usage by an
> attacker.  However constification patches are submitted all the
> time by Julia Lawall and others, and I've never seen this rationale
> spelled out in a commit message, so the assumption seems to be that
> it's common knowledge.  I could probably add something like
> 
> 	Size of pci-mid.o ELF sections:
> 
> to clarify what the numbers in the changelog refer to.

Part of this is an artifact of my workflow.  I read and review patches
in mutt, and then all I see is the body of the changelog, not the
subject line.  So in this case, all I see is:

  -.data          56
  +.data           0
  -.rodata        32
  +.rodata        88

which isn't much.  I mean, I can guess that you're reducing the size of
things.  But apparently even that isn't the reason you're doing this.
Something as simple as "make mid_pci_platform_pm const because it
never needs to be modified" might be enough.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
index a8b52dc..566ded1 100644
--- a/drivers/pci/pci-mid.c
+++ b/drivers/pci/pci-mid.c
@@ -54,7 +54,7 @@  static bool mid_pci_need_resume(struct pci_dev *dev)
 	return false;
 }
 
-static struct pci_platform_pm_ops mid_pci_platform_pm = {
+static const struct pci_platform_pm_ops mid_pci_platform_pm = {
 	.is_manageable	= mid_pci_power_manageable,
 	.set_state	= mid_pci_set_power_state,
 	.get_state	= mid_pci_get_power_state,