linux-kernel - Re: [PATCH 1/2] drivers/mfd: Add realtek pcie card reader driver

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: <201208131306.21248.arnd@arndb.de>
Date:	Mon, 13 Aug 2012 13:06:21 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	wei_wang@...lsil.com.cn
Cc:	gregkh@...uxfoundation.org, devel@...uxdriverproject.org,
	linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org,
	cjb@...top.org, sameo@...ux.intel.com, bp@...en8.de,
	aaron.lu@....com
Subject: Re: [PATCH 1/2] drivers/mfd: Add realtek pcie card reader driver

On Monday 13 August 2012, wei_wang@...lsil.com.cn wrote:
> From: Wei WANG <wei_wang@...lsil.com.cn>
> 
> Realtek PCI-E card reader driver adapts requests from upper-level
> sdmmc/memstick layer to the real physical card reader.
> 
> Signed-off-by: Wei WANG <wei_wang@...lsil.com.cn>

Hi,

This looks pretty good overall, I'm generally happy with how you have
changed the structure. Looking at some of the details now:

> +
> +static const struct pcr_ops rts5209_pcr_ops = {
> +	.extra_init_hw = rts5209_extra_init_hw,
> +	.optimize_phy = rts5209_optimize_phy,
> +	.turn_on_led = rts5209_turn_on_led,
> +	.turn_off_led = rts5209_turn_off_led,
> +	.enable_auto_blink = rts5209_enable_auto_blink,
> +	.disable_auto_blink = rts5209_disable_auto_blink,
> +};
> ...
> +
> +static const struct pcr_ops rts5229_pcr_ops = {
> +	.extra_init_hw = rts5229_extra_init_hw,
> +	.optimize_phy = rts5229_optimize_phy,
> +	.turn_on_led = rts5229_turn_on_led,
> +	.turn_off_led = rts5229_turn_off_led,
> +	.enable_auto_blink = rts5229_enable_auto_blink,
> +	.disable_auto_blink = rts5229_disable_auto_blink,
> +};

Doing this separation is fine for two or three variants, especially
since the code behind them is not very big. If it becomes likely
that there will be many more variants, you should consider moving
them into separate files.

> +void rtsx_pci_start_run(struct rtsx_pcr *pcr)
> +{
> +	/* If pci device removed, don't queue idle work any more */
> +	if (pcr->remove_pci)
> +		return;
> +
> +	if (pcr->state != PDEV_STAT_RUN) {
> +		pcr->state = PDEV_STAT_RUN;
> +		pcr->ops->enable_auto_blink(pcr);
> +	}
> +
> +	cancel_delayed_work(&pcr->idle_work);
> +	queue_delayed_work(workqueue, &pcr->idle_work, msecs_to_jiffies(200));
> +}
> +EXPORT_SYMBOL_GPL(rtsx_pci_start_run);

Let me make sure I understand the logic here: When you send a command to
the driver, you turn on the blinking LED and then set a timer to turn
it off again after 200 ms, unless another command comes in, right?

This is slightly more overhead than I think you should have here.
Doing mod_timer() on a timer function might be better if you have a lot
of commands calling into rtsx_pci_start_run. Since you currently
require a mutex to be held when disabling the blinking, you might
need to schedule a work queue without timer from that timer function
though, or alternatively change the code to not require the mutex.

	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/