| Message ID | c2b767a782f9a0e1780e5c201d245d1be5b7d78a.1476011255.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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,
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
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
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.
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
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 --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,
-.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(-)