Skip to contentSkip to conversation

usb: musb: Convert timers to use timer_setup()

PATCH In review Kees Cook 3 participants 7 messages Oct 24, 2017 – Oct 30, 2017 1 message not in the archive

Subsystems (2)
USB MUSB multipoint high speed dual-role controller
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 usb: musb: Convert timers to use timer_setup()

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.

Instead of a per-device static timer variable, a spare timer "dev_timer" is added to the musb structure for devices to use for their per-device timer.

Cc Bin Liu <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/musb/am35x.c     | 24 +++++++++++-------------
 drivers/usb/musb/blackfin.c  | 13 ++++++-------
 drivers/usb/musb/blackfin.h  |  2 --
 drivers/usb/musb/da8xx.c     | 26 ++++++++++++--------------
 drivers/usb/musb/davinci.c   | 20 +++++++++-----------
 drivers/usb/musb/musb_core.c |  6 +++---
 drivers/usb/musb/musb_core.h |  1 +
 drivers/usb/musb/musb_dsps.c |  7 ++++---
 drivers/usb/musb/tusb6010.c  | 20 +++++++++-----------
 9 files changed, 55 insertions(+), 64 deletions(-)
drivers/usb/musb/am35x.c +11 -13
index 02fbb4fe3745..2cf990e85bb2 100644
@@ -133,11 +133,9 @@ static void am35x_musb_set_vbus(struct musb *musb, int is_on)
 
 #define	POLL_SECONDS	2
 
-static struct timer_list otg_workaround;
-
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb		*musb = (void *)_musb;
+	struct musb		*musb = from_timer(musb, t, dev_timer);
 	void __iomem		*mregs = musb->mregs;
 	u8			devctl;
 	unsigned long		flags;

@@ -173,7 +171,7 @@ static void otg_timer(unsigned long _musb)
 	case OTG_STATE_B_IDLE:
 		devctl = musb_readb(mregs, MUSB_DEVCTL);
 		if (devctl & MUSB_DEVCTL_BDEVICE)
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 		else
 			musb->xceiv->otg->state = OTG_STATE_A_IDLE;
 		break;

@@ -195,12 +193,12 @@ static void am35x_musb_try_idle(struct musb *musb, unsigned long timeout)
 				musb->xceiv->otg->state == OTG_STATE_A_WAIT_BCON)) {
 		dev_dbg(musb->controller, "%s active, deleting timer\n",
 			usb_otg_state_string(musb->xceiv->otg->state));
-		del_timer(&otg_workaround);
+		del_timer(&musb->dev_timer);
 		last_timer = jiffies;
 		return;
 	}
 
-	if (time_after(last_timer, timeout) && timer_pending(&otg_workaround)) {
+	if (time_after(last_timer, timeout) && timer_pending(&musb->dev_timer)) {
 		dev_dbg(musb->controller, "Longer idle timer already pending, ignoring...\n");
 		return;
 	}

@@ -209,7 +207,7 @@ static void am35x_musb_try_idle(struct musb *musb, unsigned long timeout)
 	dev_dbg(musb->controller, "%s inactive, starting idle timer for %u ms\n",
 		usb_otg_state_string(musb->xceiv->otg->state),
 		jiffies_to_msecs(timeout - jiffies));
-	mod_timer(&otg_workaround, timeout);
+	mod_timer(&musb->dev_timer, timeout);
 }
 
 static irqreturn_t am35x_musb_interrupt(int irq, void *hci)

@@ -278,14 +276,14 @@ static irqreturn_t am35x_musb_interrupt(int irq, void *hci)
 			 */
 			musb->int_usb &= ~MUSB_INTR_VBUSERROR;
 			musb->xceiv->otg->state = OTG_STATE_A_WAIT_VFALL;
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 			WARNING("VBUS error workaround (delay coming)\n");
 		} else if (drvvbus) {
 			MUSB_HST_MODE(musb);
 			otg->default_a = 1;
 			musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;
 			portstate(musb->port1_status |= USB_PORT_STAT_POWER);
-			del_timer(&otg_workaround);
+			del_timer(&musb->dev_timer);
 		} else {
 			musb->is_active = 0;
 			MUSB_DEV_MODE(musb);

@@ -324,7 +322,7 @@ static irqreturn_t am35x_musb_interrupt(int irq, void *hci)
 
 	/* Poll for ID change */
 	if (musb->xceiv->otg->state == OTG_STATE_B_IDLE)
-		mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+		mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 
 	spin_unlock_irqrestore(&musb->lock, flags);
 

@@ -365,7 +363,7 @@ static int am35x_musb_init(struct musb *musb)
 	if (IS_ERR_OR_NULL(musb->xceiv))
 		return -EPROBE_DEFER;
 
-	setup_timer(&otg_workaround, otg_timer, (unsigned long) musb);
+	timer_setup(&musb->dev_timer, otg_timer, 0);
 
 	/* Reset the musb */
 	if (data->reset)

@@ -395,7 +393,7 @@ static int am35x_musb_exit(struct musb *musb)
 	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
 	struct omap_musb_board_data *data = plat->board_data;
 
-	del_timer_sync(&otg_workaround);
+	del_timer_sync(&musb->dev_timer);
 
 	/* Shutdown the on-chip PHY and its PLL. */
 	if (data->set_phy_power)
drivers/usb/musb/blackfin.c +6 -7
index 4418574a36a1..7c580df75dc1 100644
@@ -223,7 +223,7 @@ static irqreturn_t blackfin_interrupt(int irq, void *__hci)
 	if ((musb->xceiv->otg->state == OTG_STATE_B_IDLE
 		|| musb->xceiv->otg->state == OTG_STATE_A_WAIT_BCON) ||
 		(musb->int_usb & MUSB_INTR_DISCONNECT && is_host_active(musb))) {
-		mod_timer(&musb_conn_timer, jiffies + TIMER_DELAY);
+		mod_timer(&musb->dev_timer, jiffies + TIMER_DELAY);
 		musb->a_wait_bcon = TIMER_DELAY;
 	}
 

@@ -232,9 +232,9 @@ static irqreturn_t blackfin_interrupt(int irq, void *__hci)
 	return retval;
 }
 
-static void musb_conn_timer_handler(unsigned long _musb)
+static void musb_conn_timer_handler(struct timer_list *t)
 {
-	struct musb *musb = (void *)_musb;
+	struct musb *musb = from_timer(musb, t, dev_timer);
 	unsigned long flags;
 	u16 val;
 	static u8 toggle;

@@ -266,7 +266,7 @@ static void musb_conn_timer_handler(unsigned long _musb)
 			musb_writeb(musb->mregs, MUSB_INTRUSB, val);
 			musb->xceiv->otg->state = OTG_STATE_B_IDLE;
 		}
-		mod_timer(&musb_conn_timer, jiffies + TIMER_DELAY);
+		mod_timer(&musb->dev_timer, jiffies + TIMER_DELAY);
 		break;
 	case OTG_STATE_B_IDLE:
 		/*

@@ -310,7 +310,7 @@ static void musb_conn_timer_handler(unsigned long _musb)
 			 * shortening it, if accelerating A-plug detection
 			 * is needed in OTG mode.
 			 */
-			mod_timer(&musb_conn_timer, jiffies + TIMER_DELAY / 4);
+			mod_timer(&musb->dev_timer, jiffies + TIMER_DELAY / 4);
 		}
 		break;
 	default:

@@ -445,8 +445,7 @@ static int bfin_musb_init(struct musb *musb)
 
 	bfin_musb_reg_init(musb);
 
-	setup_timer(&musb_conn_timer, musb_conn_timer_handler,
-			(unsigned long) musb);
+	timer_setup(&musb->dev_timer, musb_conn_timer_handler, 0);
 
 	musb->xceiv->set_power = bfin_musb_set_power;
drivers/usb/musb/blackfin.h +0 -2
index c84dae546dc6..231f2d23b8a1 100644
@@ -82,6 +82,4 @@ static void dump_fifo_data(u8 *buf, u16 len)
 /* Almost 1 second */
 #define TIMER_DELAY	(1 * HZ)
 
-static struct timer_list musb_conn_timer;
-
 #endif	/* __MUSB_BLACKFIN_H__ */
drivers/usb/musb/da8xx.c +12 -14
index df88123274ca..a0bc6e7372e0 100644
@@ -138,11 +138,9 @@ static void da8xx_musb_set_vbus(struct musb *musb, int is_on)
 
 #define	POLL_SECONDS	2
 
-static struct timer_list otg_workaround;
-
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb		*musb = (void *)_musb;
+	struct musb		*musb = from_timer(musb, t, dev_timer);
 	void __iomem		*mregs = musb->mregs;
 	u8			devctl;
 	unsigned long		flags;

@@ -178,7 +176,7 @@ static void otg_timer(unsigned long _musb)
 		 * VBUSERR got reported during enumeration" cases.
 		 */
 		if (devctl & MUSB_DEVCTL_VBUS) {
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 			break;
 		}
 		musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;

@@ -201,7 +199,7 @@ static void otg_timer(unsigned long _musb)
 		musb_writeb(mregs, MUSB_DEVCTL, devctl | MUSB_DEVCTL_SESSION);
 		devctl = musb_readb(mregs, MUSB_DEVCTL);
 		if (devctl & MUSB_DEVCTL_BDEVICE)
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 		else
 			musb->xceiv->otg->state = OTG_STATE_A_IDLE;
 		break;

@@ -223,12 +221,12 @@ static void da8xx_musb_try_idle(struct musb *musb, unsigned long timeout)
 				musb->xceiv->otg->state == OTG_STATE_A_WAIT_BCON)) {
 		dev_dbg(musb->controller, "%s active, deleting timer\n",
 			usb_otg_state_string(musb->xceiv->otg->state));
-		del_timer(&otg_workaround);
+		del_timer(&musb->dev_timer);
 		last_timer = jiffies;
 		return;
 	}
 
-	if (time_after(last_timer, timeout) && timer_pending(&otg_workaround)) {
+	if (time_after(last_timer, timeout) && timer_pending(&musb->dev_timer)) {
 		dev_dbg(musb->controller, "Longer idle timer already pending, ignoring...\n");
 		return;
 	}

@@ -237,7 +235,7 @@ static void da8xx_musb_try_idle(struct musb *musb, unsigned long timeout)
 	dev_dbg(musb->controller, "%s inactive, starting idle timer for %u ms\n",
 		usb_otg_state_string(musb->xceiv->otg->state),
 		jiffies_to_msecs(timeout - jiffies));
-	mod_timer(&otg_workaround, timeout);
+	mod_timer(&musb->dev_timer, timeout);
 }
 
 static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)

@@ -297,14 +295,14 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
 			 */
 			musb->int_usb &= ~MUSB_INTR_VBUSERROR;
 			musb->xceiv->otg->state = OTG_STATE_A_WAIT_VFALL;
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 			WARNING("VBUS error workaround (delay coming)\n");
 		} else if (drvvbus) {
 			MUSB_HST_MODE(musb);
 			otg->default_a = 1;
 			musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;
 			portstate(musb->port1_status |= USB_PORT_STAT_POWER);
-			del_timer(&otg_workaround);
+			del_timer(&musb->dev_timer);
 		} else {
 			musb->is_active = 0;
 			MUSB_DEV_MODE(musb);

@@ -331,7 +329,7 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
 
 	/* Poll for ID change */
 	if (musb->xceiv->otg->state == OTG_STATE_B_IDLE)
-		mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+		mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 
 	spin_unlock_irqrestore(&musb->lock, flags);
 

@@ -393,7 +391,7 @@ static int da8xx_musb_init(struct musb *musb)
 		goto fail;
 	}
 
-	setup_timer(&otg_workaround, otg_timer, (unsigned long)musb);
+	timer_setup(&musb->dev_timer, otg_timer, 0);
 
 	/* Reset the controller */
 	musb_writel(reg_base, DA8XX_USB_CTRL_REG, DA8XX_SOFT_RESET_MASK);

@@ -431,7 +429,7 @@ static int da8xx_musb_exit(struct musb *musb)
 {
 	struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
 
-	del_timer_sync(&otg_workaround);
+	del_timer_sync(&musb->dev_timer);
 
 	phy_power_off(glue->phy);
 	phy_exit(glue->phy);
drivers/usb/musb/davinci.c +9 -11
index 52b491d3d5d8..3a7048e84e1c 100644
@@ -199,11 +199,9 @@ static void davinci_musb_set_vbus(struct musb *musb, int is_on)
 
 #define	POLL_SECONDS	2
 
-static struct timer_list otg_workaround;
-
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb		*musb = (void *)_musb;
+	struct musb		*musb = from_timer(musb, t, dev_timer);
 	void __iomem		*mregs = musb->mregs;
 	u8			devctl;
 	unsigned long		flags;

@@ -224,7 +222,7 @@ static void otg_timer(unsigned long _musb)
 		 * VBUSERR got reported during enumeration" cases.
 		 */
 		if (devctl & MUSB_DEVCTL_VBUS) {
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 			break;
 		}
 		musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;

@@ -248,7 +246,7 @@ static void otg_timer(unsigned long _musb)
 				devctl | MUSB_DEVCTL_SESSION);
 		devctl = musb_readb(mregs, MUSB_DEVCTL);
 		if (devctl & MUSB_DEVCTL_BDEVICE)
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 		else
 			musb->xceiv->otg->state = OTG_STATE_A_IDLE;
 		break;

@@ -325,14 +323,14 @@ static irqreturn_t davinci_musb_interrupt(int irq, void *__hci)
 			 */
 			musb->int_usb &= ~MUSB_INTR_VBUSERROR;
 			musb->xceiv->otg->state = OTG_STATE_A_WAIT_VFALL;
-			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+			mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 			WARNING("VBUS error workaround (delay coming)\n");
 		} else if (drvvbus) {
 			MUSB_HST_MODE(musb);
 			otg->default_a = 1;
 			musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;
 			portstate(musb->port1_status |= USB_PORT_STAT_POWER);
-			del_timer(&otg_workaround);
+			del_timer(&musb->dev_timer);
 		} else {
 			musb->is_active = 0;
 			MUSB_DEV_MODE(musb);

@@ -361,7 +359,7 @@ static irqreturn_t davinci_musb_interrupt(int irq, void *__hci)
 
 	/* poll for ID change */
 	if (musb->xceiv->otg->state == OTG_STATE_B_IDLE)
-		mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
+		mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
 
 	spin_unlock_irqrestore(&musb->lock, flags);
 

@@ -393,7 +391,7 @@ static int davinci_musb_init(struct musb *musb)
 	if (revision == 0)
 		goto fail;
 
-	setup_timer(&otg_workaround, otg_timer, (unsigned long) musb);
+	timer_setup(&musb->dev_timer, otg_timer, 0);
 
 	davinci_musb_source_power(musb, 0, 1);
 

@@ -443,7 +441,7 @@ static int davinci_musb_init(struct musb *musb)
 
 static int davinci_musb_exit(struct musb *musb)
 {
-	del_timer_sync(&otg_workaround);
+	del_timer_sync(&musb->dev_timer);
 
 	/* force VBUS off */
 	if (cpu_is_davinci_dm355()) {
drivers/usb/musb/musb_core.c +3 -3
index ff5a1a8989d5..0f34cf30cdf5 100644
@@ -485,9 +485,9 @@ void musb_load_testpacket(struct musb *musb)
 /*
  * Handles OTG hnp timeouts, such as b_ase0_brst
  */
-static void musb_otg_timer_func(unsigned long data)
+static void musb_otg_timer_func(struct timer_list *t)
 {
-	struct musb	*musb = (struct musb *)data;
+	struct musb	*musb = from_timer(musb, t, otg_timer);
 	unsigned long	flags;
 
 	spin_lock_irqsave(&musb->lock, flags);

@@ -2330,7 +2330,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	if (status < 0)
 		goto fail3;
 
-	setup_timer(&musb->otg_timer, musb_otg_timer_func, (unsigned long) musb);
+	timer_setup(&musb->otg_timer, musb_otg_timer_func, 0);
 
 	/* attach to the IRQ */
 	if (request_irq(nIrq, musb->isr, IRQF_SHARED, dev_name(dev), musb)) {
drivers/usb/musb/musb_core.h +1 -0
index 20f4614178d9..e8573975743d 100644
@@ -345,6 +345,7 @@ struct musb {
 	struct list_head	pending_list;	/* pending work list */
 
 	struct timer_list	otg_timer;
+	struct timer_list	dev_timer;
 	struct notifier_block	nb;
 
 	struct dma_controller	*dma_controller;
drivers/usb/musb/musb_dsps.c +4 -3
index f6b526606ad1..e3d0e626a5d6 100644
@@ -282,9 +282,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
 	return 0;
 }
 
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb *musb = (void *)_musb;
+	struct dsps_glue *glue = from_timer(glue, t, timer);
+	struct musb *musb = platform_get_drvdata(glue->musb);
 	struct device *dev = musb->controller;
 	unsigned long flags;
 	int err;

@@ -480,7 +481,7 @@ static int dsps_musb_init(struct musb *musb)
 		}
 	}
 
-	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
+	timer_setup(&glue->timer, otg_timer, 0);
 
 	/* Reset the musb */
 	musb_writel(reg_base, wrp->control, (1 << wrp->reset));
drivers/usb/musb/tusb6010.c +9 -11
index 4eb640c54f2c..f8fce7df654f 100644
@@ -452,11 +452,9 @@ static int tusb_musb_vbus_status(struct musb *musb)
 	return ret;
 }
 
-static struct timer_list musb_idle_timer;
-
-static void musb_do_idle(unsigned long _musb)
+static void musb_do_idle(struct timer_list *t)
 {
-	struct musb	*musb = (void *)_musb;
+	struct musb	*musb = from_timer(musb, t, dev_timer);
 	unsigned long	flags;
 
 	spin_lock_irqsave(&musb->lock, flags);

@@ -523,13 +521,13 @@ static void tusb_musb_try_idle(struct musb *musb, unsigned long timeout)
 			&& (musb->xceiv->otg->state == OTG_STATE_A_WAIT_BCON))) {
 		dev_dbg(musb->controller, "%s active, deleting timer\n",
 			usb_otg_state_string(musb->xceiv->otg->state));
-		del_timer(&musb_idle_timer);
+		del_timer(&musb->dev_timer);
 		last_timer = jiffies;
 		return;
 	}
 
 	if (time_after(last_timer, timeout)) {
-		if (!timer_pending(&musb_idle_timer))
+		if (!timer_pending(&musb->dev_timer))
 			last_timer = timeout;
 		else {
 			dev_dbg(musb->controller, "Longer idle timer already pending, ignoring\n");

@@ -541,7 +539,7 @@ static void tusb_musb_try_idle(struct musb *musb, unsigned long timeout)
 	dev_dbg(musb->controller, "%s inactive, for idle timer for %lu ms\n",
 		usb_otg_state_string(musb->xceiv->otg->state),
 		(unsigned long)jiffies_to_msecs(timeout - jiffies));
-	mod_timer(&musb_idle_timer, timeout);
+	mod_timer(&musb->dev_timer, timeout);
 }
 
 /* ticks of 60 MHz clock */

@@ -873,7 +871,7 @@ static irqreturn_t tusb_musb_interrupt(int irq, void *__hci)
 	}
 
 	if (int_src & TUSB_INT_SRC_USB_IP_CONN)
-		del_timer(&musb_idle_timer);
+		del_timer(&musb->dev_timer);
 
 	/* OTG state change reports (annoyingly) not issued by Mentor core */
 	if (int_src & (TUSB_INT_SRC_VBUS_SENSE_CHNG

@@ -982,7 +980,7 @@ static void tusb_musb_disable(struct musb *musb)
 	musb_writel(tbase, TUSB_DMA_INT_MASK, 0x7fffffff);
 	musb_writel(tbase, TUSB_GPIO_INT_MASK, 0x1ff);
 
-	del_timer(&musb_idle_timer);
+	del_timer(&musb->dev_timer);
 
 	if (is_dma_capable() && !dma_off) {
 		printk(KERN_WARNING "%s %s: dma still active\n",

@@ -1142,7 +1140,7 @@ static int tusb_musb_init(struct musb *musb)
 	musb->xceiv->set_power = tusb_draw_power;
 	the_musb = musb;
 
-	setup_timer(&musb_idle_timer, musb_do_idle, (unsigned long) musb);
+	timer_setup(&musb->dev_timer, musb_do_idle, 0);
 
 done:
 	if (ret < 0) {

@@ -1156,7 +1154,7 @@ static int tusb_musb_init(struct musb *musb)
 
 static int tusb_musb_exit(struct musb *musb)
 {
-	del_timer_sync(&musb_idle_timer);
+	del_timer_sync(&musb->dev_timer);
 	the_musb = NULL;
 
 	if (musb->board_set_power)
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
maintainer
#1 lore raw

Hi,

Quoted 23 lines · Kees Cook

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.

Instead of a per-device static timer variable, a spare timer "dev_timer" is added to the musb structure for devices to use for their per-device timer.

Cc Bin Liu <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/musb/am35x.c     | 24 +++++++++++-------------
 drivers/usb/musb/blackfin.c  | 13 ++++++-------
 drivers/usb/musb/blackfin.h  |  2 --
 drivers/usb/musb/da8xx.c     | 26 ++++++++++++--------------
 drivers/usb/musb/davinci.c   | 20 +++++++++-----------
 drivers/usb/musb/musb_core.c |  6 +++---
 drivers/usb/musb/musb_core.h |  1 +
 drivers/usb/musb/musb_dsps.c |  7 ++++---
 drivers/usb/musb/tusb6010.c  | 20 +++++++++-----------
 9 files changed, 55 insertions(+), 64 deletions(-)

[snip]

drivers/usb/musb/musb_core.h struct musb { · line 348

Quoted 9 lines
drivers/usb/musb/musb_core.h +1 -0
index 20f4614178d9..e8573975743d 100644
@@ -345,6 +345,7 @@ struct musb {
 	struct list_head	pending_list;	/* pending work list */
 
 	struct timer_list	otg_timer;
+	struct timer_list	dev_timer;

Now since dev_timer is added in struct musb for glue drivers,

Quoted 21 lines
drivers/usb/musb/musb_core.h +0 -0
 	struct notifier_block	nb;
 
 	struct dma_controller	*dma_controller;
drivers/usb/musb/musb_dsps.c +3 -2
index f6b526606ad1..e3d0e626a5d6 100644
@@ -282,9 +282,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
 	return 0;
 }
 
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb *musb = (void *)_musb;
+	struct dsps_glue *glue = from_timer(glue, t, timer);
+	struct musb *musb = platform_get_drvdata(glue->musb);
 	struct device *dev = musb->controller;
 	unsigned long flags;
 	int err;

drivers/usb/musb/musb_dsps.c static int dsps_musb_init(struct musb *musb) · line 484

Quoted 5 lines
drivers/usb/musb/musb_dsps.c +1 -1
@@ -480,7 +481,7 @@ static int dsps_musb_init(struct musb *musb)
 		}
 	}
 
-	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
+	timer_setup(&glue->timer, otg_timer, 0);

this glue->timer is duplicate. It can be removed and use musb->dev_timer instead as other glue drivers do.

Regards,
-Bin.

maintainer
#2 lore raw
Quoted 65 lines · Bin Liu

Hi,

Quoted 23 lines · Kees Cook

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.

Instead of a per-device static timer variable, a spare timer "dev_timer" is added to the musb structure for devices to use for their per-device timer.

Cc Bin Liu <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/musb/am35x.c     | 24 +++++++++++-------------
 drivers/usb/musb/blackfin.c  | 13 ++++++-------
 drivers/usb/musb/blackfin.h  |  2 --
 drivers/usb/musb/da8xx.c     | 26 ++++++++++++--------------
 drivers/usb/musb/davinci.c   | 20 +++++++++-----------
 drivers/usb/musb/musb_core.c |  6 +++---
 drivers/usb/musb/musb_core.h |  1 +
 drivers/usb/musb/musb_dsps.c |  7 ++++---
 drivers/usb/musb/tusb6010.c  | 20 +++++++++-----------
 9 files changed, 55 insertions(+), 64 deletions(-)

[snip]

drivers/usb/musb/musb_core.h struct musb { · line 348

Quoted 9 lines
drivers/usb/musb/musb_core.h +1 -0
index 20f4614178d9..e8573975743d 100644
@@ -345,6 +345,7 @@ struct musb {
 	struct list_head	pending_list;	/* pending work list */
 
 	struct timer_list	otg_timer;
+	struct timer_list	dev_timer;

Now since dev_timer is added in struct musb for glue drivers,

Quoted 24 lines
drivers/usb/musb/musb_core.h +0 -0
 	struct notifier_block	nb;
 
 	struct dma_controller	*dma_controller;
drivers/usb/musb/musb_dsps.c +3 -2
index f6b526606ad1..e3d0e626a5d6 100644
@@ -282,9 +282,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
 	return 0;
 }
 
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb *musb = (void *)_musb;
+	struct dsps_glue *glue = from_timer(glue, t, timer);
+	struct musb *musb = platform_get_drvdata(glue->musb);
 	struct device *dev = musb->controller;
 	unsigned long flags;
 	int err;

@@ -480,7 +481,7 @@ static int dsps_musb_init(struct musb *musb)
 		}
 	}
Quoted 5 lines

drivers/usb/musb/musb_dsps.c static int dsps_musb_init(struct musb *musb) · line 484

Quoted 2 lines
drivers/usb/musb/musb_dsps.c +1 -1
-	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
+	timer_setup(&glue->timer, otg_timer, 0);

this glue->timer is duplicate. It can be removed and use musb->dev_timer instead as other glue drivers do.

I do not understand your review comments at all. Can you do the fixup here to use timer_setup for the musb timer code?

thanks,

greg k-h

maintainer
#3 lore raw
Quoted 66 lines · Greg Kroah-Hartman
Quoted 65 lines · Bin Liu

Hi,

Quoted 23 lines · Kees Cook

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.

Instead of a per-device static timer variable, a spare timer "dev_timer" is added to the musb structure for devices to use for their per-device timer.

Cc Bin Liu <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/musb/am35x.c     | 24 +++++++++++-------------
 drivers/usb/musb/blackfin.c  | 13 ++++++-------
 drivers/usb/musb/blackfin.h  |  2 --
 drivers/usb/musb/da8xx.c     | 26 ++++++++++++--------------
 drivers/usb/musb/davinci.c   | 20 +++++++++-----------
 drivers/usb/musb/musb_core.c |  6 +++---
 drivers/usb/musb/musb_core.h |  1 +
 drivers/usb/musb/musb_dsps.c |  7 ++++---
 drivers/usb/musb/tusb6010.c  | 20 +++++++++-----------
 9 files changed, 55 insertions(+), 64 deletions(-)

[snip]

drivers/usb/musb/musb_core.h struct musb { · line 348

Quoted 9 lines
drivers/usb/musb/musb_core.h +1 -0
index 20f4614178d9..e8573975743d 100644
@@ -345,6 +345,7 @@ struct musb {
 	struct list_head	pending_list;	/* pending work list */
 
 	struct timer_list	otg_timer;
+	struct timer_list	dev_timer;

Now since dev_timer is added in struct musb for glue drivers,

Quoted 24 lines
drivers/usb/musb/musb_core.h +0 -0
 	struct notifier_block	nb;
 
 	struct dma_controller	*dma_controller;
drivers/usb/musb/musb_dsps.c +3 -2
index f6b526606ad1..e3d0e626a5d6 100644
@@ -282,9 +282,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
 	return 0;
 }
 
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb *musb = (void *)_musb;
+	struct dsps_glue *glue = from_timer(glue, t, timer);
+	struct musb *musb = platform_get_drvdata(glue->musb);
 	struct device *dev = musb->controller;
 	unsigned long flags;
 	int err;

@@ -480,7 +481,7 @@ static int dsps_musb_init(struct musb *musb)
 		}
 	}
Quoted 8 lines
Quoted 5 lines

drivers/usb/musb/musb_dsps.c static int dsps_musb_init(struct musb *musb) · line 484

Quoted 2 lines
drivers/usb/musb/musb_dsps.c +1 -1
-	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
+	timer_setup(&glue->timer, otg_timer, 0);

this glue->timer is duplicate. It can be removed and use musb->dev_timer instead as other glue drivers do.

I do not understand your review comments at all. Can you do the fixup here to use timer_setup for the musb timer code?

I meant something like

drivers/usb/musb/musb_dsps.c +1 -2
index f6b526606ad1..8a2abe50f2d4 100644
@@ -119,7 +119,6 @@ struct dsps_glue {
        struct platform_device *musb;   /* child musb pdev */
        const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
        int vbus_irq;                   /* optional vbus irq */
-       struct timer_list timer;        /* otg_workaround timer */
        unsigned long last_timer;    /* last timer data for each instance */
        bool sw_babble_enabled;
        void __iomem *usbss_base;

@@ -480,7 +479,7 @@ static int dsps_musb_init(struct musb *musb)
                }
        }
 
-       setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
+       timer_setup(&musb->dev_timer, otg_timer, 0);
 
        /* Reset the musb */
        musb_writel(reg_base, wrp->control, (1 << wrp->reset));

which removes the duplicated timer from dsps_glue, and uses the newly added dev_timer in struct musb.

I will send the fixup patch after I tested it.

Thanks,
-Bin.

A message that is not in the archive (sent off-list or not yet received): [email protected]

author Reviewed-by
#5 lore raw

Re: [PATCH] usb: musb: dsps: remove the duplicated timer

Quoted 5 lines · Bin Liu

Now struct musb has the timer (dev_timer) for glue drivers, so let's remove the duplicated timer defined in dsps glue driver, and use dev_timer defined in struct musb.

Signed-off-by Bin Liu <[email protected]>
Reviewed-by Kees Cook <[email protected]>

Thanks for this; it wasn't clear to me if both timers needed to exist (i.e. glue operated independently from the musb object), so I didn't make this change myself.

Greg, it probably makes sense to just take both patches. Does that work for you?

-Kees

Quoted 96 lines

---

This is the fixup patch for [1].

[1] https://marc.info/?l=linux-usb&m=150936947225302&w=2

 drivers/usb/musb/musb_dsps.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
drivers/usb/musb/musb_dsps.c +8 -10
index e3d0e626a5d6..4d95fe714f27 100644
@@ -119,7 +119,6 @@ struct dsps_glue {
        struct platform_device *musb;   /* child musb pdev */
        const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
        int vbus_irq;                   /* optional vbus irq */
-       struct timer_list timer;        /* otg_workaround timer */
        unsigned long last_timer;    /* last timer data for each instance */
        bool sw_babble_enabled;
        void __iomem *usbss_base;

@@ -149,6 +148,7 @@ struct dsps_glue {
 
 static void dsps_mod_timer(struct dsps_glue *glue, int wait_ms)
 {
+       struct musb *musb = platform_get_drvdata(glue->musb);
        int wait;
 
        if (wait_ms < 0)

@@ -156,7 +156,7 @@ static void dsps_mod_timer(struct dsps_glue *glue, int wait_ms)
        else
                wait = msecs_to_jiffies(wait_ms);
 
-       mod_timer(&glue->timer, jiffies + wait);
+       mod_timer(&musb->dev_timer, jiffies + wait);
 }
 
 /*

@@ -216,7 +216,7 @@ static void dsps_musb_disable(struct musb *musb)
        musb_writel(reg_base, wrp->coreintr_clear, wrp->usb_bitmap);
        musb_writel(reg_base, wrp->epintr_clear,
                         wrp->txep_bitmap | wrp->rxep_bitmap);
-       del_timer_sync(&glue->timer);
+       del_timer_sync(&musb->dev_timer);
 }
 
 /* Caller must take musb->lock */

@@ -230,7 +230,7 @@ static int dsps_check_status(struct musb *musb, void *unused)
        int skip_session = 0;
 
        if (glue->vbus_irq)
-               del_timer(&glue->timer);
+               del_timer(&musb->dev_timer);
 
        /*
         * We poll because DSPS IP's won't expose several OTG-critical

@@ -284,8 +284,7 @@ static int dsps_check_status(struct musb *musb, void *unused)
 
 static void otg_timer(struct timer_list *t)
 {
-       struct dsps_glue *glue = from_timer(glue, t, timer);
-       struct musb *musb = platform_get_drvdata(glue->musb);
+       struct musb *musb = from_timer(musb, t, dev_timer);
        struct device *dev = musb->controller;
        unsigned long flags;
        int err;

@@ -481,7 +480,7 @@ static int dsps_musb_init(struct musb *musb)
                }
        }
 
-       timer_setup(&glue->timer, otg_timer, 0);
+       timer_setup(&musb->dev_timer, otg_timer, 0);
 
        /* Reset the musb */
        musb_writel(reg_base, wrp->control, (1 << wrp->reset));

@@ -516,7 +515,7 @@ static int dsps_musb_exit(struct musb *musb)
        struct device *dev = musb->controller;
        struct dsps_glue *glue = dev_get_drvdata(dev->parent);
 
-       del_timer_sync(&glue->timer);
+       del_timer_sync(&musb->dev_timer);
        usb_phy_shutdown(musb->xceiv);
        phy_power_off(musb->phy);
        phy_exit(musb->phy);

@@ -1028,7 +1027,7 @@ static int dsps_suspend(struct device *dev)
                return ret;
        }
 
-       del_timer_sync(&glue->timer);
+       del_timer_sync(&musb->dev_timer);
 
        mbase = musb->ctrl_base;
        glue->context.control = musb_readl(mbase, wrp->control);
--

1.9.1

Signature · 2 lines
Kees Cook
Pixel Security
maintainer
#6 lore raw

Re: [PATCH] usb: musb: dsps: remove the duplicated timer

Quoted 7 lines · Kees Cook
Quoted 5 lines · Bin Liu

Now struct musb has the timer (dev_timer) for glue drivers, so let's remove the duplicated timer defined in dsps glue driver, and use dev_timer defined in struct musb.

Signed-off-by Bin Liu <[email protected]>
Quoted 7 lines
Reviewed-by Kees Cook <[email protected]>

Thanks for this; it wasn't clear to me if both timers needed to exist (i.e. glue operated independently from the musb object), so I didn't make this change myself.

Greg, it probably makes sense to just take both patches. Does that work for you?

Yes, I can do that, thanks.

greg k-h

maintainer Acked-by
#7 lore raw
In reply to Greg Kroah-Hartman
Quoted 66 lines · Greg Kroah-Hartman
Quoted 65 lines · Bin Liu

Hi,

Quoted 23 lines · Kees Cook

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.

Instead of a per-device static timer variable, a spare timer "dev_timer" is added to the musb structure for devices to use for their per-device timer.

Cc Bin Liu <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/musb/am35x.c     | 24 +++++++++++-------------
 drivers/usb/musb/blackfin.c  | 13 ++++++-------
 drivers/usb/musb/blackfin.h  |  2 --
 drivers/usb/musb/da8xx.c     | 26 ++++++++++++--------------
 drivers/usb/musb/davinci.c   | 20 +++++++++-----------
 drivers/usb/musb/musb_core.c |  6 +++---
 drivers/usb/musb/musb_core.h |  1 +
 drivers/usb/musb/musb_dsps.c |  7 ++++---
 drivers/usb/musb/tusb6010.c  | 20 +++++++++-----------
 9 files changed, 55 insertions(+), 64 deletions(-)

[snip]

drivers/usb/musb/musb_core.h struct musb { · line 348

Quoted 9 lines
drivers/usb/musb/musb_core.h +1 -0
index 20f4614178d9..e8573975743d 100644
@@ -345,6 +345,7 @@ struct musb {
 	struct list_head	pending_list;	/* pending work list */
 
 	struct timer_list	otg_timer;
+	struct timer_list	dev_timer;

Now since dev_timer is added in struct musb for glue drivers,

Quoted 24 lines
drivers/usb/musb/musb_core.h +0 -0
 	struct notifier_block	nb;
 
 	struct dma_controller	*dma_controller;
drivers/usb/musb/musb_dsps.c +3 -2
index f6b526606ad1..e3d0e626a5d6 100644
@@ -282,9 +282,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
 	return 0;
 }
 
-static void otg_timer(unsigned long _musb)
+static void otg_timer(struct timer_list *t)
 {
-	struct musb *musb = (void *)_musb;
+	struct dsps_glue *glue = from_timer(glue, t, timer);
+	struct musb *musb = platform_get_drvdata(glue->musb);
 	struct device *dev = musb->controller;
 	unsigned long flags;
 	int err;

@@ -480,7 +481,7 @@ static int dsps_musb_init(struct musb *musb)
 		}
 	}
Quoted 8 lines
Quoted 5 lines

drivers/usb/musb/musb_dsps.c static int dsps_musb_init(struct musb *musb) · line 484

Quoted 2 lines
drivers/usb/musb/musb_dsps.c +1 -1
-	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
+	timer_setup(&glue->timer, otg_timer, 0);

this glue->timer is duplicate. It can be removed and use musb->dev_timer instead as other glue drivers do.

I do not understand your review comments at all. Can you do the fixup here to use timer_setup for the musb timer code?

The fixup patch was sent. Here is my Acked-by for this patch.

Acked-by Bin Liu <[email protected]>

Regards,
-Bin.