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

From: Greg KH
Date: Fri Jan 10 2020 - 02:21:01 EST


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