linux-kernel - RE: [PATCH] platform/x86: dell-uart-backlight: new backlight driver for DELL AIO

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: <377b9a237f6448bba0f5a0b83c77d7bd@ausx13mpc124.AMER.DELL.COM>
Date:   Fri, 27 Oct 2017 15:52:48 +0000
From:   <Mario.Limonciello@...l.com>
To:     <acelan.kao@...onical.com>
CC:     <dvhart@...radead.org>, <andy@...radead.org>,
        <linux-kernel@...r.kernel.org>,
        <platform-driver-x86@...r.kernel.org>
Subject: RE: [PATCH] platform/x86: dell-uart-backlight: new backlight driver
 for DELL AIO

> >
> > Looking at the comment for serial8250_get_port, it sounds like the serial driver
> > doesn't really want other drivers using this function outside of suspend/resume
> callback.
> >
> > /**
> >  * serial8250_get_port - retrieve struct uart_8250_port
> >  * @line: serial line number
> >  *
> >  * This function retrieves struct uart_8250_port for the specific line.
> >  * This struct *must* *not* be used to perform a 8250 or serial core operation
> >  * which is not accessible otherwise. Its only purpose is to make the struct
> >  * accessible to the runtime-pm callbacks for context suspend/restore.
> >  * The lock assumption made here is none because runtime-pm suspend/resume
> >  * callbacks should not be invoked if there is any operation performed on the
> >  * port.
> >  */
> > http://elixir.free-
> electrons.com/linux/v4.13/source/drivers/tty/serial/8250/8250_core.c#L409
> >
> > If serial core doesn't provide an API, have you already discussed with serial
> maintainer
> > to review your approach?  It might make sense to work with serial maintainer to
> provide
> > an API for this type of usecase if it doesn't exist.
> Yes, I know the pointer returned from serial8250_get_port() becomes
> unavailable after S3.
> So, I provide a suspend/resume function try to workaround it.
> But just like the comment described, it's risky to perform serial
> operations by using the structure,
> and there is a new interface to deal with this kind of requirement, serdev.
> But serdev is introduced after v4.12, to make it work before serdev, I
> can't find other good solutions for it.

Right, I know you have a need for this to work on older kernel too, so I think
this interface as you did it makes sense on older kernel, but please do use serdev
on newer kernel and as this is upstreamed.

> 
> >> +     /* unregister acpi backlight interface */
> >> +     acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
> >
> > Since you change this when the driver is loaded, you should cache the old
> > value and restore it when the driver is unloaded too.
> 
> In this case, acpi backlight doesn't work, all the backlight
> operations would go through UART commands.
> So, it safe to remove it.

OK.
I think it would be good to include a command about this so that folks who
look at this later understand this.

Thanks,