linux-kernel - Re: [PATCH v3 2/4] mmc: omap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20131029172229.GA12193@gimli>
Date:	Tue, 29 Oct 2013 12:22:29 -0500
From:	Felipe Balbi <balbi@...com>
To:	Andreas Fenkart <afenkart@...il.com>
CC:	Felipe Balbi <balbi@...com>, Chris Ball <cjb@...top.org>,
	Tony Lindgren <tony@...mide.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Balaji T K <balajitk@...com>, Daniel Mack <zonque@...il.com>,
	<devicetree-discuss@...ts.ozlabs.org>,
	<devicetree@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	linux-omap <linux-omap@...r.kernel.org>
Subject: Re: [PATCH v3 2/4] mmc: omap_hsmmc: Enable SDIO IRQ.

Hi,

On Tue, Oct 29, 2013 at 04:06:58PM +0100, Andreas Fenkart wrote:
> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> >> index 94d6dc8..53beac4 100644
> >> --- a/drivers/mmc/host/omap_hsmmc.c
> >> +++ b/drivers/mmc/host/omap_hsmmc.c
> >> @@ -130,6 +130,7 @@ static void apply_clk_hack(struct device *dev)
> >>  #define TC_EN                        (1 << 1)
> >>  #define BWR_EN                       (1 << 4)
> >>  #define BRR_EN                       (1 << 5)
> >> +#define CIRQ_EN                      (1 << 8)
> >>  #define ERR_EN                       (1 << 15)
> >>  #define CTO_EN                       (1 << 16)
> >>  #define CCRC_EN                      (1 << 17)
> >> @@ -210,6 +211,10 @@ struct omap_hsmmc_host {
> >>       int                     reqs_blocked;
> >>       int                     use_reg;
> >>       int                     req_in_progress;
> >> +     int                     flags;
> >> +#define HSMMC_RUNTIME_SUSPENDED      (1 << 0)        /* Runtime suspended */
> >> +#define HSMMC_SDIO_IRQ_ENABLED       (1 << 1)        /* SDIO irq enabled */
> >> +
> >>       struct omap_hsmmc_next  next_data;
> >>       struct  omap_mmc_platform_data  *pdata;
> >>  };
> >> @@ -490,27 +495,40 @@ static void omap_hsmmc_stop_clock(struct omap_hsmmc_host *host)
> >>  static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
> >>                                 struct mmc_command *cmd)
> >>  {
> >> -     unsigned int irq_mask;
> >> +     u32 irq_mask = INT_EN_MASK;
> >> +     unsigned long flags;
> >>
> >>       if (host->use_dma)
> >> -             irq_mask = INT_EN_MASK & ~(BRR_EN | BWR_EN);
> >> -     else
> >> -             irq_mask = INT_EN_MASK;
> >> +             irq_mask &= ~(BRR_EN | BWR_EN);
> >
> > is this a bugfix ? should it be sent as a separate patch ?
> 
> maybe the u32, but otherwise it's just shorter... shall I drop it.

no, now I saw what you did ;-)

> >>
> >>       /* Disable timeout for erases */
> >>       if (cmd->opcode == MMC_ERASE)
> >>               irq_mask &= ~DTO_EN;
> >>
> >> +     spin_lock_irqsave(&host->irq_lock, flags);
> >
> > why do you need this new locking here ? register acesses are atomic,
> > right ? If you really do need the locking, should it be a separate patch
> > as well ?
> 
> because host->flags can be accessed from irq context as well

fair point :-)

> >>  static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host)
> >>  {
> >> -     OMAP_HSMMC_WRITE(host->base, ISE, 0);
> >> -     OMAP_HSMMC_WRITE(host->base, IE, 0);
> >> +     u32 irq_mask = 0;
> >> +     unsigned long flags;
> >> +
> >> +     spin_lock_irqsave(&host->irq_lock, flags);
> >> +     /* no transfer running, need to signal cirq if */
> >
> > if... ?
> >
> >> +     if (host->flags & HSMMC_SDIO_IRQ_ENABLED)
> >> +             irq_mask |= CIRQ_EN;
> >> +     OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
> >
> > the whole fiddling with STAT and ISE registers look very bogus to me
> > (even on current state before this patch). We shouldn't be clearing
> > pending IRQ events here, right ? We could miss IRQs due to that.
> 
> sdio_claim_host excludes multiple users and  typical users are using synchronous
> communication, issue a transfer, wait till it's done, then release the host.
> Hence when we come here, the content of ISE/STAT is a leftover from
> the previous transfer.
> Probably the intent here is to reset the registers in defined state,
> maybe it wasn't needed
> but it doesn't hurt either.
> 
> It's conservative... I don't like to change it, along the side of my
> sdio irq patches.
> If I did lots of such changes, surely I would create a regression.
> 
> On the other side If I was up to optimize the driver, then I would do this.

sure, that was an unrelated comment, just exposing some possible problem
with that approach :-)

> >> @@ -1067,8 +1085,12 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)
> >>       int status;
> >>
> >>       status = OMAP_HSMMC_READ(host->base, STAT);
> >> -     while (status & INT_EN_MASK && host->req_in_progress) {
> >> -             omap_hsmmc_do_irq(host, status);
> >> +     while (status & (INT_EN_MASK | CIRQ_EN)) {
> >
> > why don't enable CIRQ_EN in INT_EN_MASK directly ?
> 
> INT_EN_MASK is also used when bootstrapping the card in
> send_init_stream, but there
> you don't want to enable sdio irqs. So I would have to mask it there,
> so this is the smaller
> change.

true, fair enough.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)