linux-kernel - Re: [PATCH] drivers: ddcci: upstream DDCCI driver

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] [day] [month] [year] [list]

Date:   Sat, 26 Feb 2022 22:58:42 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Yusuf Khan <yusisamerican@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jason Wang <jasowang@...hat.com>,
        Michael Kelley <mikelley@...rosoft.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        gregkh <gregkh@...uxfoundation.org>, javier@...igon.com,
        Will Deacon <will@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Subject: Re: [PATCH] drivers: ddcci: upstream DDCCI driver

On Sat, Feb 26, 2022 at 11:48 AM Yusuf Khan <yusisamerican@...il.com> wrote:
> On Sat, Feb 26, 2022 at 1:39 AM Arnd Bergmann <arnd@...db.de> wrote:
> >
> > with the other ones.On Sat, Feb 26, 2022 at 3:20 AM Yusuf Khan
> > <yusisamerican@...il.com> wrote:
> > >
> >
> > If this is a backlight driver, I think it should go into
> > drivers/video/backlight/,
> > no need for a top-level subsystem.
>
> Arnd, the driver includes a backlight driver, the main part of the
> driver(ddcci.c) is a monitor communication protocol. Should
> I leave the backlight driver(ddcci-backlight.c) in drivers/video/backlight?

It's more important to decide how the user space interface should be
handled, let's decide about that first.

> > > + */
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +#include <asm-generic/fcntl.h>
> >
> > Including the asm-generic version causes the build failures. If you need
> > the contents, use <linux/fcntl.h>, otherwise leave it out.
> >
> > > +static dev_t ddcci_cdev_first;
> > > +static dev_t ddcci_cdev_next;
> > > +static dev_t ddcci_cdev_end;
> > > +static DEFINE_SEMAPHORE(core_lock);
> >
> > No new semaphores please, this should probably be a mutex.
> >
> >
> > > +struct bus_type ddcci_bus_type;
> > > +EXPORT_SYMBOL_GPL(ddcci_bus_type);
> > > +
> > > +/* Assert neccessary string array sizes  */
> > > +#ifndef sizeof_field
> > > +# define sizeof_field(t,m) FIELD_SIZEOF(t,m)
> > > +#endif
> > > +static_assert(sizeof_field(struct ddcci_device, prot) > 8);
> > > +static_assert(sizeof_field(struct ddcci_device, type) > 8);
> > > +static_assert(sizeof_field(struct ddcci_device, model) > 8);
> > > +static_assert(sizeof_field(struct ddcci_device, vendor) > 8);
> > > +static_assert(sizeof_field(struct ddcci_device, module) > 8);
> > > +
> > > +/* Internal per-i2c-client driver data */
> > > +struct ddcci_bus_drv_data {
> > > +       unsigned long quirks;
> > > +       struct i2c_client *i2c_dev;
> > > +       struct semaphore sem;
> > > +       unsigned char recv_buffer[DDCCI_RECV_BUFFER_SIZE];
> > > +};
> >
> > Same here.
> >
> > > +static const struct file_operations ddcci_fops = {
> > > +       .owner = THIS_MODULE,
> > > +       .read = ddcci_cdev_read,
> > > +       .write = ddcci_cdev_write,
> > > +       .open = ddcci_cdev_open,
> > > +       .release = ddcci_cdev_close,
> > > +       .llseek = ddcci_cdev_seek
> > > +};
> >
> > It looks like this adds low-level access to a bus that is already managed by
> > the drm (or older framebuffer) drivers. How do you prevent these two
> > from stepping on each other's toes?