All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pavel Machek <pavel@ucw.cz>
To: Johan Hovold <johan@kernel.org>
Cc: Tony Lindgren <tony@atomide.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh@kernel.org>,
	Alan Cox <gnomes@lxorguk.ukuu.org.uk>,
	Lee Jones <lee.jones@linaro.org>, Jiri Slaby <jslaby@suse.cz>,
	Merlijn Wajer <merlijn@wizzup.org>,
	Peter Hurley <peter@hurleysoftware.com>,
	Sebastian Reichel <sre@kernel.org>,
	linux-serial@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
	phone-devel@vger.kernel.org
Subject: Re: [PATCH 1/6] tty: n_gsm: Add support for serdev drivers
Date: Sun, 29 Nov 2020 21:51:45 +0100	[thread overview]
Message-ID: <20201129205144.GA15038@duo.ucw.cz> (raw)
In-Reply-To: <20200528093102.GC10358@localhost>

[-- Attachment #1: Type: text/plain, Size: 7853 bytes --]

Hi!

This is neccessary for having useful Droid 4 support, so let me try to
ressurect this.

If there's newer version (I took mine from for-5.7 branch), let me
know.

On Thu 2020-05-28 11:31:02, Johan Hovold wrote:
> On Tue, May 12, 2020 at 02:47:08PM -0700, Tony Lindgren wrote:
> > I initially though about adding the serdev support into a separate file,
> > but that will take some refactoring of n_gsm.c. And I'd like to have
> > things working first. Then later on we might want to consider splitting
> > n_gsm.c into three pieces for core, tty and serdev parts. And then maybe
> > the serdev related parts can be just moved to live under something like
> > drivers/tty/serdev/protocol/ngsm.c.
> 
> Yeah, perhaps see where this lands first, but it should probably be done
> before merging anything.

Is drivers/tty/serdev/protocol/ngsm.c acceptable place for you?

> And it doesn't really make sense exporting these interfaces without the
> actual serdev driver as they are closely tied and cannot be reviewed
> separately anyway.

Ok, I guess keeping this in series with gnss driver makes sense? That
one should be good example.

> > @@ -150,6 +152,7 @@ struct gsm_dlci {
> >  	/* Data handling callback */
> >  	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
> >  	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
> > +	struct gsm_serdev_dlci *ops; /* serdev dlci ops, if used */
> 
> Please rename the struct with a "_operations" suffix as you refer to
> this as "ops" throughout.

"struct gsm_serdev_dlci_operations" is rather long, but I can do
that; unless there's better idea? ...OTOH... yes, "ops" variable is
used for this, but it is more than "operations" structure, so the new
name is misleading. I may have to rename it back.

> > +/**
> > + * gsm_serdev_get_config - read ts 27.010 config
> > + * @gsd:	serdev-gsm instance
> > + * @c:		ts 27.010 config data
> > + *
> > + * See gsm_copy_config_values() for more information.
> 
> Please document this properly since you're exporting these
> interfaces.

Actually, let me drop this for now.

> > +/**
> > + * gsm_serdev_set_config - set ts 27.010 config
> > + * @gsd:	serdev-gsm instance
> > + * @c:		ts 27.010 config data
> > + *
> > + * See gsm_config() for more information.
> > + */
> > +int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
> > +{
> > +	struct gsm_mux *gsm;
> > +
> > +	if (!gsd || !gsd->serdev || !gsd->gsm)
> > +		return -ENODEV;
> 
> And why check for serdev here?

Having exported interfaces somehow robust looks like good thing. Do
you want me to remove it?

> > +	gsm = gsd->gsm;
> > +
> > +	if (line < 1 || line >= 63)
> 
> Line 62 is reserved as well.

Thanks, fixed.

> > +static int gsd_dlci_receive_buf(struct gsm_serdev_dlci *ops,
> > +				const unsigned char *buf,
> > +				size_t len)
> > +{
> > +	struct gsm_serdev *gsd = ops->gsd;
> 
> This looks backwards, why not pass in gsd instead?

gsm_serdev does not specify concrete dlci; we can go from dlci to gsd
but not the other way around.

...which shows that gsm_serdev_dlci is not really "operations"
structure and should not be named as such.

> > +	struct gsm_mux *gsm = dlci->gsm;
> > +	struct gsm_serdev *gsd = gsm->gsd;
> > +
> > +	if (!gsd || !dlci->ops)
> > +		return;
> > +
> > +	switch (dlci->adaption) {
> > +	case 0:
> 
> 0 isn't valid, right?
> 
> > +	case 1:
> > +		if (dlci->ops->receive_buf)
> > +			dlci->ops->receive_buf(dlci->ops, buf, len);
> > +		break;
> 
> What about adaption 2 with modem status? Why are you not reusing
> gsm_dlci_data()?

It is not needed in my application, I guess, so it would be difficult
to test.

> > +	default:
> > +		pr_warn("dlci%i adaption %i not yet implemented\n",
> > +			dlci->addr, dlci->adaption);
> 
> This needs to be rate limited. Use the dev_ versions when you can.

Ok.

> > +	mutex_lock(&dlci->mutex);
> > +	ops->gsd = gsd;
> > +	dlci->ops = ops;
> > +	dlci->modem_rx = 0;
> > +	dlci->prev_data = dlci->data;
> 
> I think this one is only used when bringing up a network interface.

prev_data is used to store data pointer, so that it can be restored on
unregister. Are you saying it is not neccessary?

> > +	dlci->data = gsd_dlci_data;
> > +	mutex_unlock(&dlci->mutex);
> > +
> > +	gsm_dlci_begin_open(dlci);
> 
> Why is this here? This should be handled when opening the serial device
> (i.e. by gsmtty_open()).

This is for in-kernel users. When gnss device is opened, this is called.

> > +	/*
> > +	 * Allow some time for dlci to move to DLCI_OPEN state. Otherwise
> > +	 * the serdev consumer driver can start sending data too early during
> > +	 * the setup, and the response will be missed by gms_queue() if we
> > +	 * still have DLCI_CLOSED state.
> > +	 */
> > +	for (retries = 10; retries > 0; retries--) {
> > +		if (dlci->state == DLCI_OPEN)
> > +			break;
> > +		msleep(100);
> > +	}
> 
> What if you time out? This should be handled properly.

Ok.

> > +static int gsd_receive_buf(struct serdev_device *serdev, const u8 *data,
> > +			   size_t count)
> > +{
> > +	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
> > +	struct gsm_mux *gsm;
> > +	const unsigned char *dp;
> > +	int i;
> > +
> > +	if (WARN_ON(!gsd))
> > +		return 0;
> 
> Probably better to take the NULL-deref. Can this ever happen?

Well, with warn_on we continue, so easier debugging. It obviously
should not happen.

> > +int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
> > +{
> > +	struct gsm_serdev_dlci *ops;
> > +	unsigned int base;
> > +	int error;
> > +
> > +	if (line < 1)
> > +		return -EINVAL;
> 
> Upper limit?

Actually, check should not be needed, as gsd_dlci_get() will check
both limits for us. Let me remove it.

> > +	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> > +	if (!ops)
> > +		return -ENOMEM;
> > +
> > +	ops->line = line;
> > +	ops->receive_buf = gsd_dlci_receive_buf;
> > +
> > +	error = gsm_serdev_register_dlci(gsd, ops);
> > +	if (error) {
> > +		kfree(ops);
> > +
> > +		return error;
> > +	}
> > +
> > +	base = mux_num_to_base(gsd->gsm);
> > +	tty_register_device(gsm_tty_driver, base + ops->line, NULL);
> 
> I would expect this to be tty_port_register_device_serdev() so that
> serdev gets initialised properly for any client devices (e.g. gnss).
> 

> > +void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line)
> > +{
> > +	struct gsm_dlci *dlci;
> > +	unsigned int base;
> > +
> > +	if (line < 1)
> > +		return;
> 
> As above.

Ok.

> > +int gsm_serdev_register_device(struct gsm_serdev *gsd)
> > +{
> > +	struct gsm_mux *gsm;
> > +	int error;
> > +
> > +	if (WARN(!gsd || !gsd->serdev || !gsd->output,
> > +		 "serdev and output must be initialized\n"))
> > +		return -EINVAL;
> 
> Just oops if the driver is buggy and fails to set essential fields.

I find such robustness helpful, but I can remove it if you insist.

> > +void gsm_serdev_unregister_device(struct gsm_serdev *gsd)
> > +{
> > +	gsm_cleanup_mux(gsd->gsm);
> > +	mux_put(gsd->gsm);
> > +	gsd->gsm = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(gsm_serdev_unregister_device);
> > +
> > +#endif	/* CONFIG_SERIAL_DEV_BUS */
> 
> It looks like you may also have a problem with tty hangups, which serdev
> does not support currently. There are multiple paths in n_gsm which can
> trigger a hangup (e.g. based on remote input) and would likely lead to a
> crash

I don't believe we need to support hangups for the Droid 4, but
obviously it would be good not to crash. But I don't know where to
start looking, do you have any hints?

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

  reply	other threads:[~2020-11-29 20:53 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-12 21:47 [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4 Tony Lindgren
2020-05-12 21:47 ` [PATCH 1/6] tty: n_gsm: Add support for serdev drivers Tony Lindgren
2020-05-13 19:24   ` Pavel Machek
2020-05-28  9:31   ` Johan Hovold
2020-11-29 20:51     ` Pavel Machek [this message]
2020-05-12 21:47 ` [PATCH 2/6] dt-bindings: serdev: ngsm: Add binding for serdev-ngsm Tony Lindgren
2020-05-28  9:38   ` Johan Hovold
2020-05-12 21:47 ` [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node Tony Lindgren
2020-05-13 19:26   ` Pavel Machek
2020-05-27 19:28   ` Rob Herring
2020-05-28  9:51     ` Johan Hovold
2021-03-05 10:46       ` Pavel Machek
2021-03-05 10:52         ` Johan Hovold
2021-03-24  1:09           ` Pavel Machek
2021-04-01  9:43         ` Johan Hovold
2020-05-12 21:47 ` [PATCH 4/6] serdev: ngsm: Add generic serdev-ngsm driver Tony Lindgren
2020-05-28 12:43   ` Johan Hovold
2020-05-12 21:47 ` [PATCH 5/6] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem Tony Lindgren
2020-05-28 13:06   ` Johan Hovold
2020-05-28 23:38     ` Tony Lindgren
2020-05-12 21:47 ` [PATCH 6/6] ARM: dts: omap4-droid4: Configure modem for serdev-ngsm Tony Lindgren
2020-05-13 19:27   ` Pavel Machek
2020-05-13 19:09 ` [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4 Pavel Machek
2020-05-14 17:31   ` Tony Lindgren
2020-05-22  9:17 ` Greg Kroah-Hartman
2020-05-25  7:44   ` Johan Hovold
2020-05-28  8:39 ` Johan Hovold
2020-05-28 12:57   ` Pavel Machek
2020-07-26  8:25   ` Pavel Machek
2020-07-28  8:36     ` Tony Lindgren
2020-12-16 22:56   ` Pavel Machek
  -- strict thread matches above, loose matches on Subject: below --
2020-04-30 17:46 [PATCHv6 " Tony Lindgren
2020-04-30 17:46 ` [PATCH 1/6] tty: n_gsm: Add support for serdev drivers Tony Lindgren
2020-05-01 20:31   ` Pavel Machek
2020-05-01 21:31     ` Tony Lindgren

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=20201129205144.GA15038@duo.ucw.cz \
    --to=pavel@ucw.cz \
    --cc=devicetree@vger.kernel.org \
    --cc=gnomes@lxorguk.ukuu.org.uk \
    --cc=gregkh@linuxfoundation.org \
    --cc=johan@kernel.org \
    --cc=jslaby@suse.cz \
    --cc=lee.jones@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=merlijn@wizzup.org \
    --cc=peter@hurleysoftware.com \
    --cc=phone-devel@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=sre@kernel.org \
    --cc=tony@atomide.com \
    --subject='Re: [PATCH 1/6] tty: n_gsm: Add support for serdev drivers' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.