Date: Sat, 24 Mar 2018 21:22:28 +0000 (UTC) From: Ian Lepore <ian@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r331497 - stable/11/sys/dev/iicbus Message-ID: <201803242122.w2OLMSF8076293@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: ian Date: Sat Mar 24 21:22:28 2018 New Revision: 331497 URL: https://svnweb.freebsd.org/changeset/base/331497 Log: MFC r328307, r328311-r328312 r328307: Fix a bug introduced with recursive bus ownership support in r321584. The recursive ownership support added in r321584 was unconditionally in effect all the time -- whenever a given i2c slave device instance tried to lock the i2c bus for exclusive use when it already owned the bus, the call returned immediately without waiting. However, many i2c slave drivers use bus ownership to enforce that only a single thread at a time can be using the slave device. The recursive locking changes broke this use case. Now there is a new flag, IIC_RECURSIVE, which can be mixed in with the other flags passed to iicbus_acquire_bus() to allow drivers to indicate when recursive locking is desired. Using the flag implies that the driver is managing concurrent access to the device by different threads in some way. This immediately fixes all existing i2c slave drivers except for the two i2c RTC drivers which use the recursive locking feature; those will be fixed in a followup commit. r328311 and r328312: Follow changes in r328307 by using new IIC_RECURSIVE flag. The driver now ensures only one thread at a time is running in the API functions (clock_gettime() and clock_settime()) by specifically requesting ownership of the i2c bus without using IIC_RECURSIVE, then it does all IO using IIC_RECURSIVE so that each individual IO operation doesn't try to re-acquire the bus. The other IO done by the driver happens at attach or intr_config_hooks time, when there can't be multiple threads running with the same device instance. So, the IIC_RECURSIVE flag can be safely ORed into the wait flags for all IO done by the driver, because it's all either done in a single-threaded environment, or protected within a block bounded by explict iicbus_acquire_bus() and iicbus_release_bus() calls. Modified: stable/11/sys/dev/iicbus/iiconf.c stable/11/sys/dev/iicbus/iiconf.h stable/11/sys/dev/iicbus/isl12xx.c stable/11/sys/dev/iicbus/nxprtc.c Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/dev/iicbus/iiconf.c ============================================================================== --- stable/11/sys/dev/iicbus/iiconf.c Sat Mar 24 20:40:16 2018 (r331496) +++ stable/11/sys/dev/iicbus/iiconf.c Sat Mar 24 21:22:28 2018 (r331497) @@ -84,7 +84,7 @@ iicbus_poll(struct iicbus_softc *sc, int how) int error; IICBUS_ASSERT_LOCKED(sc); - switch (how) { + switch (how & IIC_INTRWAIT) { case IIC_WAIT | IIC_INTR: error = mtx_sleep(sc, &sc->lock, IICPRI|PCATCH, "iicreq", 0); break; @@ -115,8 +115,14 @@ iicbus_request_bus(device_t bus, device_t dev, int how IICBUS_LOCK(sc); - while (error == 0 && sc->owner != NULL && sc->owner != dev) - error = iicbus_poll(sc, how); + for (;;) { + if (sc->owner == NULL) + break; + if ((how & IIC_RECURSIVE) && sc->owner == dev) + break; + if ((error = iicbus_poll(sc, how)) != 0) + break; + } if (error == 0) { ++sc->owncount; Modified: stable/11/sys/dev/iicbus/iiconf.h ============================================================================== --- stable/11/sys/dev/iicbus/iiconf.h Sat Mar 24 20:40:16 2018 (r331496) +++ stable/11/sys/dev/iicbus/iiconf.h Sat Mar 24 21:22:28 2018 (r331497) @@ -39,13 +39,14 @@ #define LSB 0x1 /* - * How tsleep() is called in iic_request_bus(). + * Options affecting iicbus_request_bus() */ #define IIC_DONTWAIT 0 #define IIC_NOINTR 0 #define IIC_WAIT 0x1 #define IIC_INTR 0x2 #define IIC_INTRWAIT (IIC_INTR | IIC_WAIT) +#define IIC_RECURSIVE 0x4 /* * i2c modes Modified: stable/11/sys/dev/iicbus/isl12xx.c ============================================================================== --- stable/11/sys/dev/iicbus/isl12xx.c Sat Mar 24 20:40:16 2018 (r331496) +++ stable/11/sys/dev/iicbus/isl12xx.c Sat Mar 24 21:22:28 2018 (r331497) @@ -137,18 +137,27 @@ static struct ofw_compat_data compat_data[] = { }; #endif +/* + * When doing i2c IO, indicate that we need to wait for exclusive bus ownership, + * but that we should not wait if we already own the bus. This lets us put + * iicbus_acquire_bus() calls with a non-recursive wait at the entry of our API + * functions to ensure that only one client at a time accesses the hardware for + * the entire series of operations it takes to read or write the clock. + */ +#define WAITFLAGS (IIC_WAIT | IIC_RECURSIVE) + static inline int isl12xx_read1(struct isl12xx_softc *sc, uint8_t reg, uint8_t *data) { - return (iicdev_readfrom(sc->dev, reg, data, 1, IIC_WAIT)); + return (iicdev_readfrom(sc->dev, reg, data, 1, WAITFLAGS)); } static inline int isl12xx_write1(struct isl12xx_softc *sc, uint8_t reg, uint8_t val) { - return (iicdev_writeto(sc->dev, reg, &val, 1, IIC_WAIT)); + return (iicdev_writeto(sc->dev, reg, &val, 1, WAITFLAGS)); } static void @@ -229,17 +238,23 @@ isl12xx_gettime(device_t dev, struct timespec *ts) int err; uint8_t hourmask, sreg; - /* If power failed, we can't provide valid time. */ - if ((err = isl12xx_read1(sc, ISL12XX_SR_REG, &sreg)) != 0) + /* + * Read the status and time registers. + */ + if ((err = iicbus_request_bus(sc->busdev, sc->dev, IIC_WAIT)) == 0) { + if ((err = isl12xx_read1(sc, ISL12XX_SR_REG, &sreg)) == 0) { + err = iicdev_readfrom(sc->dev, ISL12XX_SC_REG, &tregs, + sizeof(tregs), WAITFLAGS); + } + iicbus_release_bus(sc->busdev, sc->dev); + } + if (err != 0) return (err); + + /* If power failed, we can't provide valid time. */ if (sreg & ISL12XX_SR_RTCF) return (EINVAL); - /* Read the bcd time registers. */ - if ((err = iicdev_readfrom(sc->dev, ISL12XX_SC_REG, &tregs, sizeof(tregs), - IIC_WAIT)) != 0) - return (EINVAL); - /* If chip is in AM/PM mode remember that for when we set time. */ if (tregs.hour & ISL12XX_24HR_FLAG) { hourmask = ISL12xx_24HR_MASK; @@ -319,7 +334,7 @@ isl12xx_settime(device_t dev, struct timespec *ts) sreg |= ISL12XX_SR_WRTC | ISL12XX_SR_W0C_BITS; if ((err = isl12xx_write1(sc, ISL12XX_SR_REG, sreg)) == 0) { err = iicdev_writeto(sc->dev, ISL12XX_SC_REG, &tregs, - sizeof(tregs), IIC_WAIT); + sizeof(tregs), WAITFLAGS); sreg &= ~ISL12XX_SR_WRTC; isl12xx_write1(sc, ISL12XX_SR_REG, sreg); } Modified: stable/11/sys/dev/iicbus/nxprtc.c ============================================================================== --- stable/11/sys/dev/iicbus/nxprtc.c Sat Mar 24 20:40:16 2018 (r331496) +++ stable/11/sys/dev/iicbus/nxprtc.c Sat Mar 24 21:22:28 2018 (r331497) @@ -198,6 +198,15 @@ struct nxprtc_softc { #define SC_F_CPOL (1 << 0) /* Century bit means 19xx */ /* + * When doing i2c IO, indicate that we need to wait for exclusive bus ownership, + * but that we should not wait if we already own the bus. This lets us put + * iicbus_acquire_bus() calls with a non-recursive wait at the entry of our API + * functions to ensure that only one client at a time accesses the hardware for + * the entire series of operations it takes to read or write the clock. + */ +#define WAITFLAGS (IIC_WAIT | IIC_RECURSIVE) + +/* * We use the compat_data table to look up hint strings in the non-FDT case, so * define the struct locally when we don't get it from ofw_bus_subr.h. */ @@ -230,14 +239,14 @@ static int read_reg(struct nxprtc_softc *sc, uint8_t reg, uint8_t *val) { - return (iicdev_readfrom(sc->dev, reg, val, sizeof(*val), IIC_WAIT)); + return (iicdev_readfrom(sc->dev, reg, val, sizeof(*val), WAITFLAGS)); } static int write_reg(struct nxprtc_softc *sc, uint8_t reg, uint8_t val) { - return (iicdev_writeto(sc->dev, reg, &val, sizeof(val), IIC_WAIT)); + return (iicdev_writeto(sc->dev, reg, &val, sizeof(val), WAITFLAGS)); } static int @@ -264,7 +273,7 @@ read_timeregs(struct nxprtc_softc *sc, struct time_reg continue; } if ((err = iicdev_readfrom(sc->dev, sc->secaddr, tregs, - sizeof(*tregs), IIC_WAIT)) != 0) + sizeof(*tregs), WAITFLAGS)) != 0) break; } while (sc->use_timer && tregs->sec != sec); @@ -294,7 +303,7 @@ write_timeregs(struct nxprtc_softc *sc, struct time_re { return (iicdev_writeto(sc->dev, sc->secaddr, tregs, - sizeof(*tregs), IIC_WAIT)); + sizeof(*tregs), WAITFLAGS)); } static int @@ -557,14 +566,15 @@ nxprtc_gettime(device_t dev, struct timespec *ts) * bit is not set in the control reg. The latter can happen if there * was an error when setting the time. */ - if ((err = read_timeregs(sc, &tregs, &tmrcount)) != 0) { - device_printf(dev, "cannot read RTC time\n"); - return (err); + if ((err = iicbus_request_bus(sc->busdev, sc->dev, IIC_WAIT)) == 0) { + if ((err = read_timeregs(sc, &tregs, &tmrcount)) == 0) { + err = read_reg(sc, PCF85xx_R_CS1, &cs1); + } + iicbus_release_bus(sc->busdev, sc->dev); } - if ((err = read_reg(sc, PCF85xx_R_CS1, &cs1)) != 0) { - device_printf(dev, "cannot read RTC time\n"); + if (err != 0) return (err); - } + if ((tregs.sec & PCF85xx_B_SECOND_OS) || (cs1 & PCF85xx_B_CS1_STOP)) { device_printf(dev, "RTC clock not running\n"); return (EINVAL); /* hardware is good, time is not. */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201803242122.w2OLMSF8076293>