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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
>
> 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 <[email protected]> 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 <[email protected]>
>>
>> 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 5:55 PM Andrzej Hajda <[email protected]> wrote:
>
> On 18.07.2019 16:21, Arnd Bergmann wrote:
> > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> >>
> >> 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
On Thu, Jul 18, 2019 at 4:56 PM Andrzej Hajda <[email protected]> wrote:
> On 18.07.2019 16:21, Arnd Bergmann wrote:
> > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> >>
> >> 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 6:13 PM Arnd Bergmann <[email protected]> wrote:
>
> On Thu, Jul 18, 2019 at 4:56 PM Andrzej Hajda <[email protected]> wrote:
> > On 18.07.2019 16:21, Arnd Bergmann wrote:
> > > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> > >>
> > >> 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
On Thu, Jul 18, 2019 at 5:17 PM Dmitry Torokhov
<[email protected]> wrote:
> On Thu, Jul 18, 2019 at 6:13 PM Arnd Bergmann <[email protected]> wrote:
> > On Thu, Jul 18, 2019 at 4:56 PM Andrzej Hajda <[email protected]> wrote:
> > > On 18.07.2019 16:21, Arnd Bergmann wrote:
> > > > On Thu, Jul 18, 2019 at 4:16 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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
>