Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 10 Jan 2020 08:20:51 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     rishi gupta <gupt21@gmail.com>
Cc:     robh+dt@kernel.org, jslaby@suse.com, linux-serial@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/3] tty/serial: ttvys: add null modem driver
 emulating serial port
Message-ID: <20200110072051.GA124387@kroah.com>
References: <cover.1578235515.git.gupt21@gmail.com>
 <9fcb02fafd5fc9b31f3fe358b8e62b8a40ae132a.1578235515.git.gupt21@gmail.com>
 <20200106193500.GC754821@kroah.com>
 <CALUj-gsaecfZ9HN_JVAnvJijYCHK-A5qeztDLbDOSOAjTVfTeg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALUj-gsaecfZ9HN_JVAnvJijYCHK-A5qeztDLbDOSOAjTVfTeg@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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