linux-kernel - Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o8kvx987.fsf@mpe.ellerman.id.au>
Date:   Thu, 22 Oct 2020 13:52:56 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Laurent Vivier <laurent@...ier.eu>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, Joshua Thompson <funaho@...ai.org>,
        linux-serial@...r.kernel.org,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        linux-m68k@...ts.linux-m68k.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Paul Mackerras <paulus@...ba.org>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

Laurent Vivier <laurent@...ier.eu> writes:
> Le 20/10/2020 à 20:32, Greg KH a écrit :
>> On Tue, Oct 20, 2020 at 08:19:26PM +0200, Laurent Vivier wrote:
>>> Le 20/10/2020 à 19:37, Greg KH a écrit :
>>>> On Tue, Oct 20, 2020 at 06:37:41PM +0200, Laurent Vivier wrote:
>>>>> Le 20/10/2020 à 18:28, Greg KH a écrit :
>>>>>> On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
>>>>>>> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
>>>>>>> if kernel is built for Mac but not on a Mac.
>>>>>>>
>>>>>>> Signed-off-by: Laurent Vivier <laurent@...ier.eu>
>>>>>>> ---
>>>>>>>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
>>>>>>>  1 file changed, 11 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
>>>>>>> index 063484b22523..d1d2e55983c3 100644
>>>>>>> --- a/drivers/tty/serial/pmac_zilog.c
>>>>>>> +++ b/drivers/tty/serial/pmac_zilog.c
>>>>>>> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
>>>>>>>  static int __init init_pmz(void)
>>>>>>>  {
>>>>>>>  	int rc, i;
>>>>>>> +
>>>>>>> +#ifdef CONFIG_MAC
>>>>>>> +	if (!MACH_IS_MAC)
>>>>>>> +		return -ENODEV;
>>>>>>> +#endif
>>>>>>
>>>>>> Why is the #ifdef needed?
>>>>>>
>>>>>> We don't like putting #ifdef in .c files for good reasons.  Can you make
>>>>>> the api check for this work with and without that #ifdef needed?
>>>>>
>>>>> The #ifdef is needed because this file can be compiled for PowerMac and
>>>>> m68k Mac. For PowerMac, the MACH_IS_MAC is not defined, so we need the
>>>>> #ifdef.
>>>>>
>>>>> We need the MAC_IS_MAC because the same kernel can be used with several
>>>>> m68k machines, so the init_pmz can be called on a m68k machine without
>>>>> the zilog device (it's a multi-targets kernel).
>>>>>
>>>>> You can check it's the good way to do by looking inside:
>>>>>
>>>>>     drivers/video/fbdev/valkyriefb.c +317
>>>>>     drivers/macintosh/adb.c +316
>>>>>
>>>>> That are two files used by both, mac and pmac.
>>>>
>>>> Why not fix it to work properly like other arch checks are done
>>> I would be happy to do the same.
>>>
>>>> Put it in a .h file and do the #ifdef there.  Why is this "special"?
>>>
>>> I don't know.
>>>
>>> Do you mean something like:
>>>
>>> drivers/tty/serial/pmac_zilog.h
>>> ...
>>> #ifndef MACH_IS_MAC
>>> #define MACH_IS_MAC (0)
>>> #endif
>>> ...
>>>
>>> drivers/tty/serial/pmac_zilog.c
>>> ...
>>> static int __init pmz_console_init(void)
>>> {
>>>         if (!MACH_IS_MAC)
>>>                 return -ENODEV;
>>> ...
>> 
>> Yup, that would be a good start, but why is the pmac_zilog.h file
>> responsible for this?  Shouldn't this be in some arch-specific file
>> somewhere?
>
> For m68k, MACH_IS_MAC is defined in arch/m68k/include/asm/setup.h
>
> If I want to define it for any other archs I don't know in which file we
> can put it.
>
> But as m68k mac is only sharing drivers with pmac perhaps we can put
> this in arch/powerpc/include/asm/setup.h?

It doesn't really belong in there.

I'd accept a patch to create arch/powerpc/include/asm/macintosh.h, with
MACH_IS_MAC defined in there.

cheers