[PATCH] drm/bridge: fix RC_CORE dependency

[PATCH] drm/bridge: fix RC_CORE dependency

[Arnd Bergmann]

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

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.
 
-- 
2.20.0

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Andrzej Hajda]

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

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Arnd Bergmann]

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

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Andrzej Hajda]

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
>

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Arnd Bergmann]

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

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Arnd Bergmann]

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

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Andrzej Hajda]

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

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Arnd Bergmann]

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

Re: [PATCH] drm/bridge: fix RC_CORE dependency

[Andrzej Hajda]

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
>