linux-kernel - Re: [PATCH v1 2/3] tty/serial: ttvys: add null modem driver emulating serial port

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: <CALUj-gvf5vcwdj=-8Sh9BjecKwGYFJciZ7caHxbzve3XNmE-xg@mail.gmail.com>
Date:   Mon, 10 Feb 2020 20:44:31 +0530
From:   rishi gupta <gupt21@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     robh+dt@...nel.org, jslaby@...e.com, linux-serial@...r.kernel.org,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] tty/serial: ttvys: add null modem driver emulating
 serial port

Tried dev_groups approach, doesn't fit here. Please see inline.

On Fri, Jan 10, 2020 at 12:50 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Thu, Jan 09, 2020 at 02:59:59PM +0530, rishi gupta wrote:
> > > > +/* UART frame structure definitions */
> > > > +#define VS_CRTSCTS       0x0001
> > > > +#define VS_XON           0x0002
> > > > +#define VS_NONE          0X0004
> > > > +#define VS_DATA_5        0X0008
> > > > +#define VS_DATA_6        0X0010
> > > > +#define VS_DATA_7        0X0020
> > > > +#define VS_DATA_8        0X0040
> > >
> > > Why the "X"?
> > Sorry I did not understand, do you mean why VS_XON.
>
> No, I mean why the "0X0040" instead of "0x0040" like all other hex
> digits in your list of defines.
>
> > > > +static int vs_alloc_reg_one_dev(int oidx, int pidx, int rtsmap,
> > > > +                     int dtrmap, int dtropn)
> > > > +{
> > > > +     int ret;
> > > > +     struct vs_dev *vsdev;
> > > > +     struct device *dev;
> > > > +
> > > > +     /* Allocate and init virtual tty device private data */
> > > > +     vsdev = kcalloc(1, sizeof(struct vs_dev), GFP_KERNEL);
> > > > +     if (!vsdev)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     vsdev->own_tty = NULL;
> > > > +     vsdev->peer_tty = NULL;
> > > > +     vsdev->own_index = oidx;
> > > > +     vsdev->peer_index =  pidx;
> > > > +     vsdev->rts_mappings = rtsmap;
> > > > +     vsdev->dtr_mappings = dtrmap;
> > > > +     vsdev->set_odtr_at_open = dtropn;
> > > > +     vsdev->msr_reg = 0;
> > > > +     vsdev->mcr_reg = 0;
> > > > +     vsdev->waiting_msr_chg = 0;
> > > > +     vsdev->tx_paused = 0;
> > > > +     vsdev->faulty_cable = 0;
> > > > +     mutex_init(&vsdev->lock);
> > > > +
> > > > +     /* Register with tty core with specific minor number */
> > > > +     dev = tty_register_device(ttyvs_driver, oidx, NULL);
> > > > +     if (!dev) {
> > > > +             ret = -ENOMEM;
> > > > +             goto fail;
> > > > +     }
> > > > +
> > > > +     vsdev->device = dev;
> > > > +     dev_set_drvdata(dev, vsdev);
> > > > +
> > > > +     /* Create custom sysfs files for this device for events */
> > > > +     ret = sysfs_create_group(&dev->kobj, &vs_info_attr_grp);
> > >
> > > Please no.  You just raced with userspace and lost (i.e. userspace has
> > > no idea these files are present.)
> > >
> > > Please use the correct apis for this, if you _REALLY_ want special sysfs
> > > files for a tty device.
> > Any specific API would you like to suggest. I am unable to progress on
> > how to address this one.
>
> Now that you have moved things to configfs, maybe you do not need the
> sysfs files anymore?
>
> Ah your "control" sysfs files, ok, you need to set the driver's
> dev_groups variable to point to your sysfs attributes, and then the
> driver core will properly set up these files.
>
> hope this helps,
>
> greg k-h

Everything done except using dev_groups approach (full driver after
all changes https://github.com/test209/t/blob/master/ttyvs.c#L1957).

Currently to emulate parity error (or any event), user writes to a
device specific node (0 is device number):
echo "2" > /sys/devices/virtual/tty/ttyvs0/event

With dev_groups, sysfs is created (1) for driver not for devices (2)
for platform devices only

Due to (1), parsing based approach will be needed, for ex (0 is device number);
echo "0-2" > /sys/devices/platform/ttyvs-card@...vent
or
echo "0-parity" > /sys/devices/platform/ttyvs-card@...vent

Due to (2), event file will not exist on desktop systems as there will
be no device tree node; no platform device.

Original problem was user space doesn't know when
"/sys/devices/virtual/tty/ttyvs0/event" will exist.
User space gets a uevent when a device is registered with tty core.
Application must access only after this.
Is this okay in case of this particular driver.

Only after open(/dev/ttyvs0) succeeds, application should access
"/sys/devices/virtual/tty/ttyvs0/event".

Regards,
Rishi