[PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Lukas Wunner]

-.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(-)

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

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Andy Shevchenko]

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,

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Lukas Wunner]

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

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Bjorn Helgaas]

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

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Andy Shevchenko]

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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Lukas Wunner]

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

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Bjorn Helgaas]

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

[PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Lukas Wunner]

This struct never needs to be modified.  The size of pci-mid.o ELF
sections changes thusly:

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

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---

Link to Andy's ack:
https://www.spinics.net/lists/linux-pci/msg54977.html

Link to Bjorn's ack:
https://www.spinics.net/lists/linux-pci/msg56476.html

 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 c7f3408..1c4af72 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.10.2

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Bjorn Helgaas]

On Sun, Dec 04, 2016 at 03:35:55PM +0100, Lukas Wunner wrote:
> This struct never needs to be modified.  The size of pci-mid.o ELF
> sections changes thusly:
> 
> -.data          56
> +.data           0
> -.rodata        32
> +.rodata        88
> 
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Ingo, do you want this, since you've taken the previous updates?

It's trivial but doesn't apply cleanly to my tree, which makes me think
it might be based on other changes I don't have.

> ---
> 
> Link to Andy's ack:
> https://www.spinics.net/lists/linux-pci/msg54977.html
> 
> Link to Bjorn's ack:
> https://www.spinics.net/lists/linux-pci/msg56476.html
> 
>  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 c7f3408..1c4af72 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.10.2
> 
> --
> 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

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Ingo Molnar]

* Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Sun, Dec 04, 2016 at 03:35:55PM +0100, Lukas Wunner wrote:
> > This struct never needs to be modified.  The size of pci-mid.o ELF
> > sections changes thusly:
> > 
> > -.data          56
> > +.data           0
> > -.rodata        32
> > +.rodata        88
> > 
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> Ingo, do you want this, since you've taken the previous updates?
> 
> It's trivial but doesn't apply cleanly to my tree, which makes me think
> it might be based on other changes I don't have.

It applies to upstream v4.9 cleanly, so I guess it's due to other changes you have 
in your tree?

Could you take it? Me having dabbled in drivers/pci/ is more of an outlier.

Thnks,

	Ingo

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Bjorn Helgaas]

On Mon, Dec 12, 2016 at 08:27:33AM +0100, Ingo Molnar wrote:
> 
> * Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Sun, Dec 04, 2016 at 03:35:55PM +0100, Lukas Wunner wrote:
> > > This struct never needs to be modified.  The size of pci-mid.o ELF
> > > sections changes thusly:
> > > 
> > > -.data          56
> > > +.data           0
> > > -.rodata        32
> > > +.rodata        88
> > > 
> > > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > 
> > Ingo, do you want this, since you've taken the previous updates?
> > 
> > It's trivial but doesn't apply cleanly to my tree, which makes me think
> > it might be based on other changes I don't have.
> 
> It applies to upstream v4.9 cleanly, so I guess it's due to other changes you have 
> in your tree?
> 
> Could you take it? Me having dabbled in drivers/pci/ is more of an outlier.

Applied to pci/pm for v4.10.

It didn't apply cleanly to v4.9-rc1, which is what my v4.10 branches
are based on.  It applies cleanly on e8a6123e9ead
("x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_state
hook"), which was not in v4.9-rc1 but was merged via
x86-urgent-for-linus (8528d662489a).

I can't remember why we decided to put pci-mid.c in drivers/pci.  The
obviously reason is that it uses pci_platform_pm_ops, which isn't
visible outside drivers/pci, but pci-mid.c is clearly platform-
specific, and even the name "pci_platform_pm_ops" sounds like
something that should be implemented in the arch directories, so maybe
pci_platform_pm_ops should be moved to include/linux/pci.h.

I have strange sense of déjà vu writing this, so sorry if I'm
repeating something.

Bjorn

Re: [PATCH] x86/platform/intel-mid: Constify mid_pci_platform_pm

[Andy Shevchenko]

On Mon, 2016-12-12 at 10:01 -0600, Bjorn Helgaas wrote:
> On Mon, Dec 12, 2016 at 08:27:33AM +0100, Ingo Molnar wrote:

> I can't remember why we decided to put pci-mid.c in drivers/pci.

In the first place there were no such comments like below.

>   The
> obviously reason is that it uses pci_platform_pm_ops, which isn't
> visible outside drivers/pci, but pci-mid.c is clearly platform-
> specific, and even the name "pci_platform_pm_ops" sounds like
> something that should be implemented in the arch directories, so maybe
> pci_platform_pm_ops should be moved to include/linux/pci.h.

You are repeating yourself here. Last time I have referred to another
user of that struct which doesn't belong to arch, but to drivers/acpi.
And my opinion here is to keep things for now, until some common
solution would be developed.

> I have strange sense of déjà vu writing this, so sorry if I'm
> repeating something.

Yes, that's right.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy