| Message ID | 20190718134240.2265724-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: fix RC_CORE dependency | expand |
Hi Arnd, On 18.07.2019 15:42, Arnd Bergmann wrote: > Using 'imply' causes a new problem, as it allows the case of > CONFIG_INPUT=m with RC_CORE=y, which fails to link: > > drivers/media/rc/rc-main.o: In function `ir_do_keyup': > rc-main.c:(.text+0x2b4): undefined reference to `input_event' > drivers/media/rc/rc-main.o: In function `rc_repeat': > rc-main.c:(.text+0x350): undefined reference to `input_event' > drivers/media/rc/rc-main.o: In function `rc_allocate_device': > rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' > > Add a 'depends on' that allows building both with and without > CONFIG_RC_CORE, but disallows combinations that don't link. > > Fixes: 5023cf32210d ("drm/bridge: make remote control optional") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Proper solution has been already merged via input tree[1]. [1]: https://lore.kernel.org/lkml/CAKdAkRTGXNbUsuKASNGLfwUwC7Asod9K5baYLPWPU7EX-42-yA@mail.gmail.com/ Regards Andrzej > --- > drivers/gpu/drm/bridge/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index f64c91defdc3..70a8ed2505aa 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -85,8 +85,8 @@ config DRM_SIL_SII8620 > tristate "Silicon Image SII8620 HDMI/MHL bridge" > depends on OF > select DRM_KMS_HELPER > + depends on RC_CORE || !RC_CORE > imply EXTCON > - imply RC_CORE > help > Silicon Image SII8620 HDMI/MHL bridge chip driver. >
On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > Hi Arnd, > > On 18.07.2019 15:42, Arnd Bergmann wrote: > > Using 'imply' causes a new problem, as it allows the case of > > CONFIG_INPUT=m with RC_CORE=y, which fails to link: > > > > drivers/media/rc/rc-main.o: In function `ir_do_keyup': > > rc-main.c:(.text+0x2b4): undefined reference to `input_event' > > drivers/media/rc/rc-main.o: In function `rc_repeat': > > rc-main.c:(.text+0x350): undefined reference to `input_event' > > drivers/media/rc/rc-main.o: In function `rc_allocate_device': > > rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' > > > > Add a 'depends on' that allows building both with and without > > CONFIG_RC_CORE, but disallows combinations that don't link. > > > > Fixes: 5023cf32210d ("drm/bridge: make remote control optional") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > Proper solution has been already merged via input tree[1]. > > > [1]: > https://lore.kernel.org/lkml/CAKdAkRTGXNbUsuKASNGLfwUwC7Asod9K5baYLPWPU7EX-42-yA@mail.gmail.com/ At that link, I only see the patch that caused the regression, not the solution. Are you sure it's fixed? Arnd
On 18.07.2019 16:21, Arnd Bergmann wrote: > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <a.hajda@samsung.com> wrote: >> Hi Arnd, >> >> On 18.07.2019 15:42, Arnd Bergmann wrote: >>> Using 'imply' causes a new problem, as it allows the case of >>> CONFIG_INPUT=m with RC_CORE=y, which fails to link: >>> >>> drivers/media/rc/rc-main.o: In function `ir_do_keyup': >>> rc-main.c:(.text+0x2b4): undefined reference to `input_event' >>> drivers/media/rc/rc-main.o: In function `rc_repeat': >>> rc-main.c:(.text+0x350): undefined reference to `input_event' >>> drivers/media/rc/rc-main.o: In function `rc_allocate_device': >>> rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' >>> >>> Add a 'depends on' that allows building both with and without >>> CONFIG_RC_CORE, but disallows combinations that don't link. >>> >>> Fixes: 5023cf32210d ("drm/bridge: make remote control optional") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >> Proper solution has been already merged via input tree[1]. >> >> >> [1]: >> https://lore.kernel.org/lkml/CAKdAkRTGXNbUsuKASNGLfwUwC7Asod9K5baYLPWPU7EX-42-yA@mail.gmail.com/ > At that link, I only see the patch that caused the regression, not > the solution. Are you sure it's fixed? Ups, you are right, I though you are fixing what this patch attempted to fix :) Anyway, we want to avoid dependency on RC_CORE - this driver does not require it, but with RC_CORE it has additional features. Maybe "imply INPUT" would help? Regards Andrzej > > Arnd >
On Thu, Jul 18, 2019 at 4:56 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > On 18.07.2019 16:21, Arnd Bergmann wrote: > > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > >> Hi Arnd, > >> > >> On 18.07.2019 15:42, Arnd Bergmann wrote: > >>> Using 'imply' causes a new problem, as it allows the case of > >>> CONFIG_INPUT=m with RC_CORE=y, which fails to link: > >>> > >>> drivers/media/rc/rc-main.o: In function `ir_do_keyup': > >>> rc-main.c:(.text+0x2b4): undefined reference to `input_event' > >>> drivers/media/rc/rc-main.o: In function `rc_repeat': > >>> rc-main.c:(.text+0x350): undefined reference to `input_event' > >>> drivers/media/rc/rc-main.o: In function `rc_allocate_device': > >>> rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' > >>> > >>> Add a 'depends on' that allows building both with and without > >>> CONFIG_RC_CORE, but disallows combinations that don't link. > >>> > >>> Fixes: 5023cf32210d ("drm/bridge: make remote control optional") > >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > >> > >> Proper solution has been already merged via input tree[1]. > >> > >> > >> [1]: > >> https://lore.kernel.org/lkml/CAKdAkRTGXNbUsuKASNGLfwUwC7Asod9K5baYLPWPU7EX-42-yA@mail.gmail.com/ > > At that link, I only see the patch that caused the regression, not > > the solution. Are you sure it's fixed? > > > Ups, you are right, I though you are fixing what this patch attempted to > fix :) > > Anyway, we want to avoid dependency on RC_CORE - this driver does not > require it, but with RC_CORE it has additional features. Right, that's what my patch does: if RC_CORE is disabled, you can still set DRM_SIL_SII8620=y, but if RC_CORE=m, DRM_SIL_SII8620 can only be =m or =n. > Maybe "imply INPUT" would help? No, that would make it worse. Device drivers really have no business turning on other subsystems. Arnd
On Thu, Jul 18, 2019 at 5:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > On 18.07.2019 16:21, Arnd Bergmann wrote: > > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > >> Hi Arnd, > >> > >> On 18.07.2019 15:42, Arnd Bergmann wrote: > >>> Using 'imply' causes a new problem, as it allows the case of > >>> CONFIG_INPUT=m with RC_CORE=y, which fails to link: > >>> > >>> drivers/media/rc/rc-main.o: In function `ir_do_keyup': > >>> rc-main.c:(.text+0x2b4): undefined reference to `input_event' > >>> drivers/media/rc/rc-main.o: In function `rc_repeat': > >>> rc-main.c:(.text+0x350): undefined reference to `input_event' > >>> drivers/media/rc/rc-main.o: In function `rc_allocate_device': > >>> rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' > >>> > >>> Add a 'depends on' that allows building both with and without > >>> CONFIG_RC_CORE, but disallows combinations that don't link. > >>> > >>> Fixes: 5023cf32210d ("drm/bridge: make remote control optional") > >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > >> > >> Proper solution has been already merged via input tree[1]. > >> > >> > >> [1]: > >> https://lore.kernel.org/lkml/CAKdAkRTGXNbUsuKASNGLfwUwC7Asod9K5baYLPWPU7EX-42-yA@mail.gmail.com/ > > At that link, I only see the patch that caused the regression, not > > the solution. Are you sure it's fixed? > > > Ups, you are right, I though you are fixing what this patch attempted to > fix :) > > Anyway, we want to avoid dependency on RC_CORE - this driver does not > require it, but with RC_CORE it has additional features. > > Maybe "imply INPUT" would help? No, it won't. I am sorry, I should have looked closer, but as written, drivers/gpu/drm/bridge/sil-sii8620.c has a hard dependency on the RC core and "imply" was the wrong solution for this, we need "depends on RC_CORE". If we want to make RC support optional than we should stub out paths that use RC_CORE (such as sii8620_init_rcp_input_dev()) and guard them by "#ifdef CONFIG_RC_CORE". Then we could keep "imply" on RC_CORE. I am surprised though that imply allows violating the constraint on implied symbols, as RC_CORE has straight "depends on" for INPUT. Thanks.
On Thu, Jul 18, 2019 at 6:13 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Jul 18, 2019 at 4:56 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > On 18.07.2019 16:21, Arnd Bergmann wrote: > > > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > >> Hi Arnd, > > >> > > >> On 18.07.2019 15:42, Arnd Bergmann wrote: > > >>> Using 'imply' causes a new problem, as it allows the case of > > >>> CONFIG_INPUT=m with RC_CORE=y, which fails to link: > > >>> > > >>> drivers/media/rc/rc-main.o: In function `ir_do_keyup': > > >>> rc-main.c:(.text+0x2b4): undefined reference to `input_event' > > >>> drivers/media/rc/rc-main.o: In function `rc_repeat': > > >>> rc-main.c:(.text+0x350): undefined reference to `input_event' > > >>> drivers/media/rc/rc-main.o: In function `rc_allocate_device': > > >>> rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' > > >>> > > >>> Add a 'depends on' that allows building both with and without > > >>> CONFIG_RC_CORE, but disallows combinations that don't link. > > >>> > > >>> Fixes: 5023cf32210d ("drm/bridge: make remote control optional") > > >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > >> > > >> Proper solution has been already merged via input tree[1]. > > >> > > >> > > >> [1]: > > >> https://lore.kernel.org/lkml/CAKdAkRTGXNbUsuKASNGLfwUwC7Asod9K5baYLPWPU7EX-42-yA@mail.gmail.com/ > > > At that link, I only see the patch that caused the regression, not > > > the solution. Are you sure it's fixed? > > > > > > Ups, you are right, I though you are fixing what this patch attempted to > > fix :) > > > > Anyway, we want to avoid dependency on RC_CORE - this driver does not > > require it, but with RC_CORE it has additional features. > > Right, that's what my patch does: if RC_CORE is disabled, you can > still set DRM_SIL_SII8620=y, but if RC_CORE=m, DRM_SIL_SII8620 > can only be =m or =n. > > > Maybe "imply INPUT" would help? > > No, that would make it worse. Device drivers really have no business > turning on other subsystems. > OK, in the meantime I will redo the branch by dropping the sil-sii8620.c Kconfig changes and also drop all "imply" business from applespi driver as they give us more trouble than they are worth. We do not have "imply" for i801_smbus for Symaptics SMBUS mode and it works fine. It it distro's task to configure the kernel properly. Thanks.
On Thu, Jul 18, 2019 at 5:17 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Thu, Jul 18, 2019 at 6:13 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Jul 18, 2019 at 4:56 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > > On 18.07.2019 16:21, Arnd Bergmann wrote: > > > > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > > >> Proper solution has been already merged via input tree[1]. > > > >> > > > >> > > > >> [1]: > > > >> https://lore.kernel.org/lkml/CAKdAkRTGXNbUsuKASNGLfwUwC7Asod9K5baYLPWPU7EX-42-yA@mail.gmail.com/ > > > > At that link, I only see the patch that caused the regression, not > > > > the solution. Are you sure it's fixed? > > > > > > > > > Ups, you are right, I though you are fixing what this patch attempted to > > > fix :) > > > > > > Anyway, we want to avoid dependency on RC_CORE - this driver does not > > > require it, but with RC_CORE it has additional features. > > > > Right, that's what my patch does: if RC_CORE is disabled, you can > > still set DRM_SIL_SII8620=y, but if RC_CORE=m, DRM_SIL_SII8620 > > can only be =m or =n. > > > > > Maybe "imply INPUT" would help? > > > > No, that would make it worse. Device drivers really have no business > > turning on other subsystems. > > > > OK, in the meantime I will redo the branch by dropping the > sil-sii8620.c Kconfig changes and also drop all "imply" business from > applespi driver as they give us more trouble than they are worth. We > do not have "imply" for i801_smbus for Symaptics SMBUS mode and it > works fine. It it distro's task to configure the kernel properly. Thanks! I think the "drm/bridge: make remote control optional" patch is fine with my fixup, the IS_ENABLED() checks take care of the case where RC_CORE is unavailable, and the 'depends on RC_CORE || !RC_CORE' line takes care of the RC_CORE=m case. I suppose Ronald could send a replacement patch with my fixup after the merge window. Arnd
On 18.07.2019 15:42, Arnd Bergmann wrote: > Using 'imply' causes a new problem, as it allows the case of > CONFIG_INPUT=m with RC_CORE=y, which fails to link: I have reviewed dependencies and I wonder how such configuration is possible at all. RC_CORE depends on INPUT (at least on today's next branch) so if INPUT=m then RC_CORE should be either n either m, am I right? Arnd, are there unknown to me changes in RC/INPUT dependencies? Regards Andrzej > > drivers/media/rc/rc-main.o: In function `ir_do_keyup': > rc-main.c:(.text+0x2b4): undefined reference to `input_event' > drivers/media/rc/rc-main.o: In function `rc_repeat': > rc-main.c:(.text+0x350): undefined reference to `input_event' > drivers/media/rc/rc-main.o: In function `rc_allocate_device': > rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' > > Add a 'depends on' that allows building both with and without > CONFIG_RC_CORE, but disallows combinations that don't link. > > Fixes: 5023cf32210d ("drm/bridge: make remote control optional") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/bridge/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index f64c91defdc3..70a8ed2505aa 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -85,8 +85,8 @@ config DRM_SIL_SII8620 > tristate "Silicon Image SII8620 HDMI/MHL bridge" > depends on OF > select DRM_KMS_HELPER > + depends on RC_CORE || !RC_CORE > imply EXTCON > - imply RC_CORE > help > Silicon Image SII8620 HDMI/MHL bridge chip driver. >
On Fri, Jul 19, 2019 at 9:01 AM Andrzej Hajda <a.hajda@samsung.com> wrote: > > On 18.07.2019 15:42, Arnd Bergmann wrote: > > Using 'imply' causes a new problem, as it allows the case of > > CONFIG_INPUT=m with RC_CORE=y, which fails to link: > > > I have reviewed dependencies and I wonder how such configuration is > possible at all. > > RC_CORE depends on INPUT (at least on today's next branch) so if INPUT=m > then RC_CORE should be either n either m, am I right? Right. > Arnd, are there unknown to me changes in RC/INPUT dependencies? I think this is 'imply' behaving oddly when we have conflicting requirements: - INPUT=m forces RC_CORE to be =m or =n - DRM_SIL_SII8620=y asks RC_CORE to be =y unless it cannot be enabled Kconfig decided to make this RC_CORE=y, which caused the link failure. Making it RC_CORE=m however would not work either because then we'd get a link failure from the sii8620 driver to rc_core. so a pure 'imply' cannot work here, and we need a dependency, one of: a) depends on INPUT || !INPUT select RC_CORE if INPUT b) depends on RC_CORE || !RC_CORE b) is what othe drivers use, e.g. SMS_SDIO_DRV Arnd
On 19.07.2019 10:33, Arnd Bergmann wrote: > On Fri, Jul 19, 2019 at 9:01 AM Andrzej Hajda <a.hajda@samsung.com> wrote: >> On 18.07.2019 15:42, Arnd Bergmann wrote: >>> Using 'imply' causes a new problem, as it allows the case of >>> CONFIG_INPUT=m with RC_CORE=y, which fails to link: >> >> I have reviewed dependencies and I wonder how such configuration is >> possible at all. >> >> RC_CORE depends on INPUT (at least on today's next branch) so if INPUT=m >> then RC_CORE should be either n either m, am I right? > Right. > >> Arnd, are there unknown to me changes in RC/INPUT dependencies? > I think this is 'imply' behaving oddly when we have conflicting requirements: > > - INPUT=m forces RC_CORE to be =m or =n > - DRM_SIL_SII8620=y asks RC_CORE to be =y unless it cannot be enabled > > Kconfig decided to make this RC_CORE=y, which caused the link > failure. Making it RC_CORE=m however would not work either because > then we'd get a link failure from the sii8620 driver to rc_core. > > so a pure 'imply' cannot work here, and we need a dependency, one of: > > a) > depends on INPUT || !INPUT > select RC_CORE if INPUT > > b) depends on RC_CORE || !RC_CORE > > b) is what othe drivers use, e.g. SMS_SDIO_DRV OK, thanks for explanation, really weird. I though about imply as "weak dependency", but it is "weak select" with drawbacks of select. Anyway I am surprised that Kconfig did not complain about contradictory requirements on RC_CORE symbol. Regards Andrzej > > Arnd >
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index f64c91defdc3..70a8ed2505aa 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -85,8 +85,8 @@ config DRM_SIL_SII8620 tristate "Silicon Image SII8620 HDMI/MHL bridge" depends on OF select DRM_KMS_HELPER + depends on RC_CORE || !RC_CORE imply EXTCON - imply RC_CORE help Silicon Image SII8620 HDMI/MHL bridge chip driver.
Using 'imply' causes a new problem, as it allows the case of CONFIG_INPUT=m with RC_CORE=y, which fails to link: drivers/media/rc/rc-main.o: In function `ir_do_keyup': rc-main.c:(.text+0x2b4): undefined reference to `input_event' drivers/media/rc/rc-main.o: In function `rc_repeat': rc-main.c:(.text+0x350): undefined reference to `input_event' drivers/media/rc/rc-main.o: In function `rc_allocate_device': rc-main.c:(.text+0x90c): undefined reference to `input_allocate_device' Add a 'depends on' that allows building both with and without CONFIG_RC_CORE, but disallows combinations that don't link. Fixes: 5023cf32210d ("drm/bridge: make remote control optional") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/gpu/drm/bridge/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)