linux-kernel - Re: [PATCH] char: Added support for u-blox 6 i2c gps driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150115112953.27cc5934@lxorguk.ukuu.org.uk>
Date:	Thu, 15 Jan 2015 11:29:53 +0000
From:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
To:	Felipe Tonello <eu@...ipetonello.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] char: Added support for u-blox 6 i2c gps driver

Which kernel did you see the write_room oops ? and I'll double check its
all fixed.

> >> +     ublox_gps_i2c_client = client;
> >> +     ublox_gps_filp = NULL;
> >> +     ublox_gps_tty_port = NULL;
> >> +     ublox_gps_is_open = false;
> >
> > There are some other i2c based tty drivers in the kernel - notably those
> > in drivers/tty/serial that use the uart layer to deal with most of the
> > awkward locking cases.
> >
> > You can do it by hand but it's fairly hairy (see
> > drivers/mmc/card/sdio_uart.c, so it might be simplest to tweak the driver
> > to use the uart layer. You don't really gain much from it for your driver
> > except easier locking - but the locking is rather handy.
> >
> > Alan
> 
> Ok.
> 
> The thing is that: I wrote this driver to work with only one gps
> module, because that's my configuration here. I cannot really test
> multiple i2c gps at the same time. If you guys really want a driver
> that works for multiple gps drivers, I cannot test it.

It isn't just about multiple GPS devices, it's about locking. What stops
things being unloaded or reloaded and freeing memory you are still using.
Things happen in parallel. If you have an i2c hot unplug happen as
you are running your worker thread for example this would occur

	Device still plugged in
               if (!ublox_gps_i2c_client)
	       {False so we continue}
	Device unplugged
               Use ublock_gps_i2c_client
               *Kaboom*

There are two ways to deal with that

1. Don't free the resources until the device is not being used (so while
the hardware may have walked the memory and pointers are still valid)

2. take a lock before checking, drop it after you finish using the
object. Take the same lock when destroying it.

The kernel mostly does #1, in part because the second case tends to be
hard to get right and avoid deadlocks.

I'm much less worried about the single device parts of it. The static
values you have are fairly easy to deal with I think

ublox_gps_filp isn't needed (its only used for bogus tests)
ublox_gps_is_open isn't needed (it's only used for the open test)

ublox_gps_i2c_client belongs as a pointer in your tty_data
ublox_gps_tty_port belongs as the client pointer in your i2c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/