| Message ID | 20221215165453.1864836-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | tty: serial: qcom_geni: avoid duplicate struct member init | expand |
Hi, On Thu, Dec 15, 2022 at 8:55 AM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > When -Woverride-init is enabled in a build, gcc points out that > qcom_geni_serial_pm_ops contains conflicting initializers: > > drivers/tty/serial/qcom_geni_serial.c:1586:20: error: initialized field overwritten [-Werror=override-init] > 1586 | .restore = qcom_geni_serial_sys_hib_resume, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/tty/serial/qcom_geni_serial.c:1586:20: note: (near initialization for 'qcom_geni_serial_pm_ops.restore') > drivers/tty/serial/qcom_geni_serial.c:1587:17: error: initialized field overwritten [-Werror=override-init] > 1587 | .thaw = qcom_geni_serial_sys_hib_resume, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Open-code the initializers with the version that was already used, > and use the pm_sleep_ptr() method to deal with unused ones, > in place of the __maybe_unused annotation. > > Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/tty/serial/qcom_geni_serial.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index b487823f0e61..03dda47184d9 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -1516,7 +1516,7 @@ static int qcom_geni_serial_remove(struct platform_device *pdev) > return 0; > } > > -static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev) > +static int qcom_geni_serial_sys_suspend(struct device *dev) Officially the removal of "__maybe_unused" could be a totally different patch, right? SET_SYSTEM_SLEEP_PM_OPS() already eventually used pm_sleep_ptr() even without your change, so the removal of these tags is unrelated to the rest of your change, right? > { > struct qcom_geni_serial_port *port = dev_get_drvdata(dev); > struct uart_port *uport = &port->uport; > @@ -1533,7 +1533,7 @@ static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev) > return uart_suspend_port(private_data->drv, uport); > } > > -static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev) > +static int qcom_geni_serial_sys_resume(struct device *dev) > { > int ret; > struct qcom_geni_serial_port *port = dev_get_drvdata(dev); > @@ -1581,10 +1581,12 @@ static int qcom_geni_serial_sys_hib_resume(struct device *dev) > } > > static const struct dev_pm_ops qcom_geni_serial_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_sys_suspend, > - qcom_geni_serial_sys_resume) > - .restore = qcom_geni_serial_sys_hib_resume, > - .thaw = qcom_geni_serial_sys_hib_resume, > + .suspend = pm_sleep_ptr(qcom_geni_serial_sys_suspend), > + .resume = pm_sleep_ptr(qcom_geni_serial_sys_resume), > + .freeze = pm_sleep_ptr(qcom_geni_serial_sys_suspend), > + .poweroff = pm_sleep_ptr(qcom_geni_serial_sys_suspend), > + .restore = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), > + .thaw = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), Personally, the order you listed them is less intuitive than the order that SET_SYSTEM_SLEEP_PM_OPS() lists functions. IMO it's better to consistently alternate matching suspend/resume functions. ;-) Both of those are nits, so I'm also fine with: Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Thu, Dec 15, 2022, at 21:46, Doug Anderson wrote: > On Thu, Dec 15, 2022 at 8:55 AM Arnd Bergmann <arnd@kernel.org> wrote: >> index b487823f0e61..03dda47184d9 100644 >> --- a/drivers/tty/serial/qcom_geni_serial.c >> +++ b/drivers/tty/serial/qcom_geni_serial.c >> @@ -1516,7 +1516,7 @@ static int qcom_geni_serial_remove(struct platform_device *pdev) >> return 0; >> } >> >> -static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev) >> +static int qcom_geni_serial_sys_suspend(struct device *dev) > > Officially the removal of "__maybe_unused" could be a totally > different patch, right? SET_SYSTEM_SLEEP_PM_OPS() already eventually > used pm_sleep_ptr() even without your change, so the removal of these > tags is unrelated to the rest of your change, right? It's a little more complicated: SYSTEM_SLEEP_PM_OPS() uses pm_sleep_ptr() to avoid the need for a __maybe_unused(). The depreacated SET_SYSTEM_SLEEP_PM_OPS() is based on SYSTEM_SLEEP_PM_OPS() these days, but still retains the old semantics of using an empty definition without CONFIG_PM_SLEEP, so it still leaves the function unused as far as gcc is concerned. There could be an intermediate step of open-coding the SET_SYSTEM_SLEEP_PM_OPS(), but that would result in the rather silly static const struct dev_pm_ops qcom_geni_serial_pm_ops = { #ifdef CONFIG_PM_SLEEP .suspend = qcom_geni_serial_sys_suspend, .resume = qcom_geni_serial_sys_resume, .freeze = qcom_geni_serial_sys_suspend, .poweroff = qcom_geni_serial_sys_suspend, #endif .restore = qcom_geni_serial_sys_hib_resume, .thaw = qcom_geni_serial_sys_hib_resume, } which makes no sense to me, as I think you either want all the members or none of them. >> static const struct dev_pm_ops qcom_geni_serial_pm_ops = { >> - SET_SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_sys_suspend, >> - qcom_geni_serial_sys_resume) >> - .restore = qcom_geni_serial_sys_hib_resume, >> - .thaw = qcom_geni_serial_sys_hib_resume, >> + .suspend = pm_sleep_ptr(qcom_geni_serial_sys_suspend), >> + .resume = pm_sleep_ptr(qcom_geni_serial_sys_resume), >> + .freeze = pm_sleep_ptr(qcom_geni_serial_sys_suspend), >> + .poweroff = pm_sleep_ptr(qcom_geni_serial_sys_suspend), >> + .restore = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), >> + .thaw = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), > > Personally, the order you listed them is less intuitive than the order > that SET_SYSTEM_SLEEP_PM_OPS() lists functions. IMO it's better to > consistently alternate matching suspend/resume functions. ;-) Makes sense. I kept the order that we already had here, but I could redo this patch if anyone cares. > Both of those are nits, so I'm also fine with: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thanks, Arnd
Hi, On Fri, Dec 16, 2022 at 2:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Dec 15, 2022, at 21:46, Doug Anderson wrote: > > On Thu, Dec 15, 2022 at 8:55 AM Arnd Bergmann <arnd@kernel.org> wrote: > > >> index b487823f0e61..03dda47184d9 100644 > >> --- a/drivers/tty/serial/qcom_geni_serial.c > >> +++ b/drivers/tty/serial/qcom_geni_serial.c > >> @@ -1516,7 +1516,7 @@ static int qcom_geni_serial_remove(struct platform_device *pdev) > >> return 0; > >> } > >> > >> -static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev) > >> +static int qcom_geni_serial_sys_suspend(struct device *dev) > > > > Officially the removal of "__maybe_unused" could be a totally > > different patch, right? SET_SYSTEM_SLEEP_PM_OPS() already eventually > > used pm_sleep_ptr() even without your change, so the removal of these > > tags is unrelated to the rest of your change, right? > > It's a little more complicated: SYSTEM_SLEEP_PM_OPS() uses pm_sleep_ptr() > to avoid the need for a __maybe_unused(). The depreacated > SET_SYSTEM_SLEEP_PM_OPS() is based on SYSTEM_SLEEP_PM_OPS() these days, > but still retains the old semantics of using an empty definition > without CONFIG_PM_SLEEP, so it still leaves the function unused as > far as gcc is concerned. > > There could be an intermediate step of open-coding the > SET_SYSTEM_SLEEP_PM_OPS(), but that would result in the rather > silly > > static const struct dev_pm_ops qcom_geni_serial_pm_ops = { > #ifdef CONFIG_PM_SLEEP > .suspend = qcom_geni_serial_sys_suspend, > .resume = qcom_geni_serial_sys_resume, > .freeze = qcom_geni_serial_sys_suspend, > .poweroff = qcom_geni_serial_sys_suspend, > #endif > .restore = qcom_geni_serial_sys_hib_resume, > .thaw = qcom_geni_serial_sys_hib_resume, > } > > which makes no sense to me, as I think you either want > all the members or none of them. Ah, I guess I didn't trace through all the similarly named macros quite correctly. ;-) Thanks for explaining. > >> static const struct dev_pm_ops qcom_geni_serial_pm_ops = { > >> - SET_SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_sys_suspend, > >> - qcom_geni_serial_sys_resume) > >> - .restore = qcom_geni_serial_sys_hib_resume, > >> - .thaw = qcom_geni_serial_sys_hib_resume, > >> + .suspend = pm_sleep_ptr(qcom_geni_serial_sys_suspend), > >> + .resume = pm_sleep_ptr(qcom_geni_serial_sys_resume), > >> + .freeze = pm_sleep_ptr(qcom_geni_serial_sys_suspend), > >> + .poweroff = pm_sleep_ptr(qcom_geni_serial_sys_suspend), > >> + .restore = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), > >> + .thaw = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), > > > > Personally, the order you listed them is less intuitive than the order > > that SET_SYSTEM_SLEEP_PM_OPS() lists functions. IMO it's better to > > consistently alternate matching suspend/resume functions. ;-) > > Makes sense. I kept the order that we already had here, but > I could redo this patch if anyone cares. I wouldn't worry about it. -Doug
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index b487823f0e61..03dda47184d9 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -1516,7 +1516,7 @@ static int qcom_geni_serial_remove(struct platform_device *pdev) return 0; } -static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev) +static int qcom_geni_serial_sys_suspend(struct device *dev) { struct qcom_geni_serial_port *port = dev_get_drvdata(dev); struct uart_port *uport = &port->uport; @@ -1533,7 +1533,7 @@ static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev) return uart_suspend_port(private_data->drv, uport); } -static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev) +static int qcom_geni_serial_sys_resume(struct device *dev) { int ret; struct qcom_geni_serial_port *port = dev_get_drvdata(dev); @@ -1581,10 +1581,12 @@ static int qcom_geni_serial_sys_hib_resume(struct device *dev) } static const struct dev_pm_ops qcom_geni_serial_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_sys_suspend, - qcom_geni_serial_sys_resume) - .restore = qcom_geni_serial_sys_hib_resume, - .thaw = qcom_geni_serial_sys_hib_resume, + .suspend = pm_sleep_ptr(qcom_geni_serial_sys_suspend), + .resume = pm_sleep_ptr(qcom_geni_serial_sys_resume), + .freeze = pm_sleep_ptr(qcom_geni_serial_sys_suspend), + .poweroff = pm_sleep_ptr(qcom_geni_serial_sys_suspend), + .restore = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), + .thaw = pm_sleep_ptr(qcom_geni_serial_sys_hib_resume), }; static const struct of_device_id qcom_geni_serial_match_table[] = {