Re: [PATCH 1/6] tty: n_gsm: Add support for serdev drivers

[Pavel Machek <pavel@ucw.cz>]

[-- 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 --]