linux-kernel - RE: [Patch 2/3] via-sdmmc: via-sdmmc.c

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Dec 2008 17:45:42 +0800
From:	<JosephChan@....com.tw>
To:	<arnd@...db.de>
Cc:	<linux-kernel@...r.kernel.org>
Subject: RE: [Patch 2/3] via-sdmmc: via-sdmmc.c

Thanks, Arnd.

> > +	pm_pcictrl_reg->pcitmoctrl_reg = readb(addrbase + 
> PCITMOCTRL_REG); }
> 
> Since you already define the data structure for the save 
> area, how about using it for the register accesses as well? 
> You could drop all the PCI*_REG macro definitions and do
> 
> struct pcictrlreg __iomem *pcr = 
> vcrdr_chip->pcictrl_mmiobase; pm_pcictrl_reg->pcisdclk_reg = 
> readb(&pcr->pcisdclk_reg);
> 
> Of course, your code is doing the same things effectively and 
> entirely ok here.

We'll modify the code according to your suggestions..

> > +	}
> > +
> > +	via_set_ddma(host->chip, virt_to_phys(host->ddmabuf), 
> count, dir, 
> > +0);
> 
> You can't use virt_to_phys here, since the physical address 
> might not be the same as the bus address, in case you are 
> using an IOMMU. The correct way to do it would be to drop the 
> memcpy to the internal buffer, and do a
> dma_map_sg() to get the bus address.
> 

We'll try to modify the code like below, but need more tests.

In via_sdc_preparedata() function:

    int sg_cnt;

    sg_cnt = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len, (data->flags & MMC_DATA_READ) ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
    WARN_ON(sg_cnt != 1);
    via_set_ddma(host->chip, sg_dma_address(data->sg), sg_dma_len(data->sg), (data->flags & MMC_DATA_READ) ? DMA_FROM_DEVICE : DMA_TO_DEVICE );

In via_sdc_finish_data() function:

    dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len, (data->flags & MMC_DATA_READ) ? DMA_FROM_DEVICE : DMA_TO_DEVICE);



> > +
> > +	sd_status &= ~(SD_STS_CMD_MASK | SD_STS_DATA_MASK);
> > +	if (sd_status) {
> > +		pr_err("%s: Unexpected interrupt 0x%x\n",
> > +		       mmc_hostname(sdhost->mmc), sd_status);
> > +		writew(sd_status, addrbase + SDSTATUS_REG);
> > +	}
> 
> What are your criteria for deciding which events to handle in 
> interrupt context or in tasklet context? Are some of them 
> extremely performance critical?

The criteria are: 
1. Because the SD card detect operations need delay about 1 ms, so it should not be implemented in interrupt context. So we implement it by via_sdc_tasklet_card.
2. The STSTATUS_REG register must be reset quickly, so it should be implemented in interrupt context.
3. In order to finish one “request” from the mmc_block layer quickly, all operations (that can finished quickly) before the end of the “request” should be implemented in interrupt context.


> If you can do all of them in a single tasklet function that 
> you simply schedule every time without the spinlock, you 
> don't need to disable interrupts every time you access a 
> register but can instead use spin_lock_bh.
> 
> To go even further, you could use a work queue instead of the 
> tasklet and use a mutex instead of the spinlock.

We will try to optimize the ISR function. 

> > +	writel(0x0, addrbase + SDINTMASK_REG);
> > +	mdelay(1);
> 
> Since this function runs in task context, you should use 
> msleep() here, not mdelay().

OK, we will replae it. Thanks.