Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2012-06-18 at 15:42 +0530, Jassi Brar wrote:
> On 18 June 2012 13:41, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
> >>
> >> Explicitly maintaining HDMI phy power state using a flag is prone to
> >> race and un-necessary when we have a zero-cost alternative of checking
> >> the state before trying to set it.
> >
> > Why would reading the value from the register be any less racy than
> > keeping it in memory?
> >
> Racy in the sense that h/w doesn't always hop states according to what
> a "state" variable would expect it to.

But I don't think the status register is any better. If I'm not
mistaken, when you set the phy pwr to, say, TXON, the status register
will still show the old value. The status register will be right only
after the HW is actually at TXON state.

In this case the hdmi_set_phy_pwr() function will anyway wait until the
HW has changed states, so both approaches should work just fine.

> Also in this case, phy_tx_enabled modification is unprotected in
> ti_hdmi_4xxx_phy_disable().

So is hdmi_set_phy_pwr(). Note that I'm not saying the current approach
is not racy, but your patch doesn't make it any less racy.

> BTW, coming to think about it, I am not sure what we need the
> spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
> can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
> expensive and is already unprotected elsewhere.

It's needed when enabling the hdmi output. In phy_enable() the irq is
requested first, and then the phy_enable() runs hdmi_check_hpd_state().
So there's a chance to run hdmi_check_hpd_state() from both
hpd-interrupt and phy_enable() at the same time.

The hdmi_set_phy_pwr() is not called in many places, but I think there's
indeed a problem there. It is called after free_irq(), but I think
(guess) the irq handler could still be running after free_irq. So those
should be protected by the same spinlock too.

 Tomi

Attachment: signature.asc
Description: This is a digitally signed message part


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux