ptp: Avoid deadlocks in the programmable pin code.

The PTP Hardware Clock (PHC) subsystem offers an API for configuring
programmable pins.  User space sets or gets the settings using ioctls,
and drivers verify dialed settings via a callback.  Drivers may also
query pin settings by calling the ptp_find_pin() method.

Although the core subsystem protects concurrent access to the pin
settings, the implementation places illogical restrictions on how
drivers may call ptp_find_pin().  When enabling an auxiliary function
via the .enable(on=1) callback, drivers may invoke the pin finding
method, but when disabling with .enable(on=0) drivers are not
permitted to do so.  With the exception of the mv88e6xxx, all of the
PHC drivers do respect this restriction, but still the locking pattern
is both confusing and unnecessary.

This patch changes the locking implementation to allow PHC drivers to
freely call ptp_find_pin() from their .enable() and .verify()
callbacks.

V2 ChangeLog:
- fixed spelling in the kernel doc
- add Vladimir's tested by tag

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Reported-by: Yangbo Lu <yangbo.lu@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index ac72a324fcd1458932fa405587a50216af10c66a..415c273109829959c28316a62b271ac3bfde83a3 100644 (file)
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -628,7 +628,7 @@ static void recalibrate(struct dp83640_clock *clock)
        u16 cal_gpio, cfg0, evnt, ptp_trig, trigger, val;
 
        trigger = CAL_TRIGGER;
-       cal_gpio = 1 + ptp_find_pin(clock->ptp_clock, PTP_PF_PHYSYNC, 0);
+       cal_gpio = 1 + ptp_find_pin_unlocked(clock->ptp_clock, PTP_PF_PHYSYNC, 0);
        if (cal_gpio < 1) {
                pr_err("PHY calibration pin not available - PHY is not calibrated.");
                return;

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 9d72ab593f13fde1d29862461cbc2a63fb322baa..93d574faf1fed421d338b0e5cb14f023967e78d3 100644 (file)
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -175,7 +175,10 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
                }
                req.type = PTP_CLK_REQ_EXTTS;
                enable = req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0;
+               if (mutex_lock_interruptible(&ptp->pincfg_mux))
+                       return -ERESTARTSYS;
                err = ops->enable(ops, &req, enable);
+               mutex_unlock(&ptp->pincfg_mux);
                break;
 
        case PTP_PEROUT_REQUEST:
@@ -206,7 +209,10 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
                }
                req.type = PTP_CLK_REQ_PEROUT;
                enable = req.perout.period.sec || req.perout.period.nsec;
+               if (mutex_lock_interruptible(&ptp->pincfg_mux))
+                       return -ERESTARTSYS;
                err = ops->enable(ops, &req, enable);
+               mutex_unlock(&ptp->pincfg_mux);
                break;
 
        case PTP_ENABLE_PPS:
@@ -217,7 +223,10 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
                        return -EPERM;
                req.type = PTP_CLK_REQ_PPS;
                enable = arg ? 1 : 0;
+               if (mutex_lock_interruptible(&ptp->pincfg_mux))
+                       return -ERESTARTSYS;
                err = ops->enable(ops, &req, enable);
+               mutex_unlock(&ptp->pincfg_mux);
                break;
 
        case PTP_SYS_OFFSET_PRECISE:

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index ac1f2bf9e8884f8400d13820757952ec0252726b..acabbe72e55eb94a0049225aac7a5e405a6653c7 100644 (file)
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -348,7 +348,6 @@ int ptp_find_pin(struct ptp_clock *ptp,
        struct ptp_pin_desc *pin = NULL;
        int i;
 
-       mutex_lock(&ptp->pincfg_mux);
        for (i = 0; i < ptp->info->n_pins; i++) {
                if (ptp->info->pin_config[i].func == func &&
                    ptp->info->pin_config[i].chan == chan) {
@@ -356,12 +355,26 @@ int ptp_find_pin(struct ptp_clock *ptp,
                        break;
                }
        }
-       mutex_unlock(&ptp->pincfg_mux);
 
        return pin ? i : -1;
 }
 EXPORT_SYMBOL(ptp_find_pin);
 
+int ptp_find_pin_unlocked(struct ptp_clock *ptp,
+                         enum ptp_pin_function func, unsigned int chan)
+{
+       int result;
+
+       mutex_lock(&ptp->pincfg_mux);
+
+       result = ptp_find_pin(ptp, func, chan);
+
+       mutex_unlock(&ptp->pincfg_mux);
+
+       return result;
+}
+EXPORT_SYMBOL(ptp_find_pin_unlocked);
+
 int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay)
 {
        return kthread_mod_delayed_work(ptp->kworker, &ptp->aux_work, delay);

diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index c64a1ef87240713fc9df9f2db4309205994f7227..121a7eda459351cf215fb497dbf4e22bdadf8f21 100644 (file)
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -223,6 +223,12 @@ extern s32 scaled_ppm_to_ppb(long ppm);
 /**
  * ptp_find_pin() - obtain the pin index of a given auxiliary function
  *
+ * The caller must hold ptp_clock::pincfg_mux.  Drivers do not have
+ * access to that mutex as ptp_clock is an opaque type.  However, the
+ * core code acquires the mutex before invoking the driver's
+ * ptp_clock_info::enable() callback, and so drivers may call this
+ * function from that context.
+ *
  * @ptp:    The clock obtained from ptp_clock_register().
  * @func:   One of the ptp_pin_function enumerated values.
  * @chan:   The particular functional channel to find.
@@ -233,6 +239,19 @@ extern s32 scaled_ppm_to_ppb(long ppm);
 int ptp_find_pin(struct ptp_clock *ptp,
                 enum ptp_pin_function func, unsigned int chan);
 
+/**
+ * ptp_find_pin_unlocked() - wrapper for ptp_find_pin()
+ *
+ * This function acquires the ptp_clock::pincfg_mux mutex before
+ * invoking ptp_find_pin().  Instead of using this function, drivers
+ * should most likely call ptp_find_pin() directly from their
+ * ptp_clock_info::enable() method.
+ *
+ */
+
+int ptp_find_pin_unlocked(struct ptp_clock *ptp,
+                         enum ptp_pin_function func, unsigned int chan);
+
 /**
  * ptp_schedule_worker() - schedule ptp auxiliary work
  *