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]

Message-Id: <200812181349.55601.arnd@arndb.de>
Date:	Thu, 18 Dec 2008 13:49:55 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	JosephChan@....com.tw, Ben Dooks <ben-linux@...ff.org>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [Patch 2/3] via-sdmmc: via-sdmmc.c

On Wednesday 17 December 2008, JosephChan@....com.tw wrote:

> > 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..

Ben Dooks didn't like that suggestion, and I don't care that
much, so you may want to leave this one alone.

> 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);

AFAICT, it would be perfectly fine for sg_cnt to be larger than 1,
just not smaller. I don't understand yet how your hardware would
deal with this though.

>     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 );

Ideally, the via_set_ddma function would be able to program the scatterlist
into the hardware registers directly. If this doesn't work, you may
be able to look over all elements in the list manually. If this doesn't
work either, you will have to go back to your original code, and replace
the virt_to_phys conversion with dma_map_single()/dma_unmap_single().

> > 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.

I would argue that 1ms is too long for tasklet (softirq) context as well,
and this should better be moved to a work queue.

> 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.

It's still hard to tell what 'quickly' means here. There is a latency for
entering work queues or tasklets, but if the system is idle, that should
not be noticable. OTOH, if the system is busy, it may be worthwhile doing
something more important before working on the MMC workqueue.

It's probably something you should measure. If you don't find a significant
slowdown by moving to a work queue implementation, I would recommend doing
that in order to simplify the driver.

	Arnd <><
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/