On Fri, Apr 07, 2017 at 11:02:33AM +0800, Shawn Guo wrote: > On Thu, Apr 06, 2017 at 01:12:51PM -0400, Sean Paul wrote: > > On Thu, Apr 06, 2017 at 11:01:10PM +0800, Shawn Guo wrote: > > > +static int zx_vga_connector_get_modes(struct drm_connector *connector) > > > +{ > > > + struct zx_vga *vga = to_zx_vga(connector); > > > + unsigned long flags; > > > + struct edid *edid; > > > + int ret; > > > + > > > + /* > > > + * Clear both detection bits to switch I2C bus from device > > > + * detecting to EDID reading. > > > + */ > > > + spin_lock_irqsave(&vga->lock, flags); > > > + zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, 0); > > > + spin_unlock_irqrestore(&vga->lock, flags); > > > + > > > + edid = drm_get_edid(connector, &vga->ddc->adap); > > > + if (!edid) > > > + return 0; > > > + > > > + /* > > > + * As edid reading succeeds, device must be connected, so we set > > > + * up detection bit for unplug interrupt here. > > > + */ > > > + spin_lock_irqsave(&vga->lock, flags); > > > + zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_HAS_DEVICE); > > > + spin_unlock_irqrestore(&vga->lock, flags); > > > + > > > + drm_mode_connector_update_edid_property(connector, edid); > > > + ret = drm_add_edid_modes(connector, edid); > > > + kfree(edid); > > > + > > > + return ret; > > > +} > > <snip> > > > > +static irqreturn_t zx_vga_irq_handler(int irq, void *dev_id) > > > +{ > > > + struct zx_vga *vga = dev_id; > > > + u32 status; > > > + > > > + status = zx_readl(vga->mmio + VGA_I2C_STATUS); > > > + > > > + /* Clear interrupt status */ > > > + zx_writel_mask(vga->mmio + VGA_I2C_STATUS, VGA_CLEAR_IRQ, > > > + VGA_CLEAR_IRQ); > > > + > > > + if (status & VGA_DEVICE_CONNECTED) { > > > + /* > > > + * Since VGA_DETECT_SEL bits need to be reset for switching DDC > > > + * bus from device detection to EDID read, rather than setting > > > + * up HAS_DEVICE bit here, we need to do that in .get_modes > > > + * hook for unplug detecting after EDID read succeeds. > > > + */ > > > + vga->connected = true; > > > + return IRQ_WAKE_THREAD; > > > + } > > > + > > > + if (status & VGA_DEVICE_DISCONNECTED) { > > > + spin_lock(&vga->lock); > > > + zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, > > > + VGA_DETECT_SEL_NO_DEVICE); > > > + spin_unlock(&vga->lock); > > > > I think you still have the race here. If you get a disconnect between get_edid > > successfully finishing, and resetting the DETECT_SEL register, you will end up > > with the device being disconnected and DETECT_SEL == VGA_DETECT_SEL_HAS_DEVICE. > > > > In order to close this, you'll need to hold the lock across the edid read. > > We cannot hold a spin lock across the EDID read procedure, which does > sleep. Yeah, this is a pretty common problem. We usually use a mutex in conjunction with a work function to handle this type of thing. > > When you suggested to have a lock for DETECT_SEL register access, I > thought we are accessing it in a read-modify-write way and thus agreed > to add a lock. However, I just found that it's not the case actually. > All the accesses to the register are single direct write. > > And here is my understanding about the condition you described above. > Once DETECT_SEL register gets reset (both bits cleared), the hardware > switches DDC bus for EDID read, and hotplug detection will stop working > right away (this is how ZTE hardware works). That said, if we get a > disconnect before drm_get_edid() successfully finishes, two points: > > - The irq handler will not be triggered, so it will not get a chance to > update DETECT_SEL register. Ah, this was the missing piece I needed. I hadn't realized that the first VGA_AUTO_DETECT_SEL write in get_modes disabled the irq. > - The drm_get_edid() fails, and .get_modes returns with both detect bits > kept cleared. > > I think what we can do better in this case is that we should set the > device state into disconnected, before returning from .get_modes, > something like the code below. But still, I do not see we need a lock > for that. Yep, agreed. Can you also please add to your comment in the !edid case, something like: * Locking is not required here since the only other place to write this register * (and connected var) is the irq handler. The irq handler is disabled because * we've cleared AUTO_DETECT_SEL above. Thanks for walking me through this. Sean > > Shawn > > static int zx_vga_connector_get_modes(struct drm_connector *connector) > { > struct zx_vga *vga = to_zx_vga(connector); > unsigned long flags; > struct edid *edid; > int ret; > > /* > * Clear both detection bits to switch I2C bus from device > * detecting to EDID reading. > */ > zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, 0); > > edid = drm_get_edid(connector, &vga->ddc->adap); > if (!edid) { > /* > * If EDID reading fails, we set the device state into > * disconnected. > */ > zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, > VGA_DETECT_SEL_NO_DEVICE); > vga->connected = false; > return 0; > } > > /* > * As edid reading succeeds, device must be connected, so we set > * up detection bit for unplug interrupt here. > */ > zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_HAS_DEVICE); > > drm_mode_connector_update_edid_property(connector, edid); > ret = drm_add_edid_modes(connector, edid); > kfree(edid); > > return ret; > } -- Sean Paul, Software Engineer, Google / Chromium OS -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html