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, lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 2/3] tty/serial: ttvys: add null modem driver emulating serial port
Date: Wed, 12 Feb 2020 12:02:18 -0800 [thread overview]
Message-ID: <20200212200218.GA2081271@kroah.com> (raw)
In-Reply-To: <CALUj-gvf5vcwdj=-8Sh9BjecKwGYFJciZ7caHxbzve3XNmE-xg@mail.gmail.com>
On Mon, Feb 10, 2020 at 08:44:31PM +0530, rishi gupta wrote:
> Tried dev_groups approach, doesn't fit here. Please see inline.
>
> On Fri, Jan 10, 2020 at 12:50 PM Greg KH <gregkh@linuxfoundation.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
Huh? It's there for devices.
> (2) for platform devices only
No, should work for any device type, as the logic is in the driver core.
> Due to (1), parsing based approach will be needed, for ex (0 is device number);
> echo "0-2" > /sys/devices/platform/ttyvs-card@0/event
> or
> echo "0-parity" > /sys/devices/platform/ttyvs-card@0/event
No, the 0- should not be needed, it should be a device-specific file.
> Due to (2), event file will not exist on desktop systems as there will
> be no device tree node; no platform device.
I don't understand, this file belongs in the tty device that you have
created, not in any other device.
> Original problem was user space doesn't know when
> "/sys/devices/virtual/tty/ttyvs0/event" will exist.
And if you set the devices group up properly, the sysfs file will be
created when the device is created.
> User space gets a uevent when a device is registered with tty core.
> Application must access only after this.
Yes, but you will race in creation of your file with userspace unless
you tell the core to do this properly.
> Is this okay in case of this particular driver.
No.
thanks,
greg k-h
next prev parent reply other threads:[~2020-02-12 20:02 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-06 7:21 [PATCH v1 0/3] Add virtual serial null modem emulation driver Rishi Gupta
2020-01-06 7:21 ` [PATCH v1 1/3] dt-bindings: ttyvs: document serial null modem driver dt bindings Rishi Gupta
2020-01-06 7:21 ` [PATCH v1 2/3] tty/serial: ttvys: add null modem driver emulating serial port Rishi Gupta
2020-01-06 19:35 ` Greg KH
2020-01-09 9:29 ` rishi gupta
2020-01-10 7:20 ` Greg KH
2020-01-10 7:38 ` rishi gupta
2020-01-10 7:42 ` Greg KH
2020-02-10 15:14 ` rishi gupta
2020-02-12 20:02 ` Greg KH [this message]
2020-01-06 21:20 ` kbuild test robot
2020-01-06 7:21 ` [PATCH v1 3/3] tty: documentation: abi: add ttyvs null modem driver sysfs nodes Rishi Gupta
2020-01-06 19:29 ` Greg KH
2020-01-09 9:22 ` rishi gupta
2020-01-06 19:28 ` [PATCH v1 0/3] Add virtual serial null modem emulation driver Greg KH
2020-01-09 9:22 ` rishi gupta
2020-01-06 20:23 ` H. Peter Anvin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200212200218.GA2081271@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=gupt21@gmail.com \
--cc=jslaby@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).