| 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
| ||
|
Message-ID: <20110616112355.GG23739@shale.localdomain>
Date: Thu, 16 Jun 2011 14:23:55 +0300
From: Dan Carpenter <error27@...il.com>
To: edwin_rong@...lsil.com.cn
Cc: gregkh@...e.de, devel@...uxdriverproject.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Realtek cr: Add autosuspend function.
This patch means we poll 20 times a second and test to see if we
have gone for 1000 polls without doing anything if so then we sleep.
Is there a way to get the same effect without the polling? Like
maybe set a timer for 50 seconds in the future and every time we do
something then push the timer further into the future?
On Thu, Jun 16, 2011 at 03:10:35PM +0800, edwin_rong@...lsil.com.cn wrote:
> +config REALTEK_AUTOPM
> + bool "Realtek Card Reader autosuspend support"
> + depends on USB_STORAGE_REALTEK
> + default y
>
This should probably depend on CONFIG_PM_RUNTIME as well.
> struct rts51x_chip {
> - u16 vendor_id;
> - u16 product_id;
> - char max_lun;
> + u16 vendor_id;
> + u16 product_id;
> + char max_lun;
There are about 50 lines of whitespace changes in this patch, but
those should be separated out and sent as a second patch. I'm not
going to go through the patch and point out all 50 lines
individually.
> /* flag definition */
> @@ -97,9 +139,28 @@ struct rts51x_chip {
> #define RTS51X_GET_VID(chip) ((chip)->vendor_id)
> #define RTS51X_GET_PID(chip) ((chip)->product_id)
>
> +#define VENDOR_ID(chip) ((chip)->status->vid)
> +#define PRODUCT_ID(chip) ((chip)->status->pid)
> +#ifdef CONFIG_REALTEK_AUTOPM
> +#define FW_VERSION(chip) ((chip)->status->fw_ver)
> +#else
> #define FW_VERSION(chip) ((chip)->status[0].fw_ver)
These definitions of FW_VERSION() produce the same thing, so we only
need one. The original definition is better because chip->status
is an array, it's allocated in init_realtek_cr()
size = (chip->max_lun + 1) * sizeof(struct rts51x_status);
chip->status = kzalloc(size, GFP_KERNEL);
VENDOR_ID() and PRODUCT_ID() should be defined the same way.
> -#define wait_timeout_x(task_state, msecs) \
> -do { \
> - set_current_state((task_state)); \
> - schedule_timeout((msecs) * HZ / 1000); \
> -} while (0)
> -
> -#define wait_timeout(msecs) \
> - wait_timeout_x(TASK_INTERRUPTIBLE, (msecs))
> -
Removing these unused macros is a nice cleanup, but it should go in
a separate patch.
> +static int fw5895_init(struct us_data *us)
> +{
> + struct rts51x_chip *chip = (struct rts51x_chip *)(us->extra);
> + int retval;
> + u8 val;
> +
> + US_DEBUGP("-- %s --\n", __func__);
> +
> + if ((PRODUCT_ID(chip) != 0x0158) || (FW_VERSION(chip) != 0x5895)) {
> + US_DEBUGP("Not the specified device, return immediately!\n");
> + return STATUS_SUCCESS;
Should we really return SUCCESS here? No one actually checks the
return value of this function so I suppose it doesn't matter either
way.
> + }
> +
> + retval = rts51x_read_mem(us, 0xFD6F, &val, 1);
> + if (retval != STATUS_SUCCESS) {
> + US_DEBUGP("Read memory fail\n");
> + RETURN(STATUS_FAIL);
There are two debug output lines in a row here. One is enough.
> +void rts51x_polling(struct work_struct *work)
> +{
> + struct rts51x_chip *chip = container_of(work, struct rts51x_chip,
> + rts51x_delayed_work.work);
> + struct us_data *us = chip->us;
> +
> + /* lock the device pointers */
> + mutex_lock(&(us->dev_mutex));
^ ^
These parens are not needed. (My static checker sucks and
it complains that the mutex_lock() and mutex_unlock() don't
match because one has parens and one doesn't).
> +
> + US_DEBUGP("%s: <---\n", __func__);
> +
> + switch (rts51x_get_stat(chip)) {
> + case RTS51X_STAT_INIT:
> + break;
> + case RTS51X_STAT_RUN:
> + chip->idle_counter = 0;
> + break;
> + case RTS51X_STAT_IDLE:{
> + if (chip->idle_counter <
> + (ss_delay * 1000 / POLLING_INTERVAL)) {
> + US_DEBUGP("%s: idle_counter ++\n", __func__);
> + chip->idle_counter++;
> + break;
> + }
^
Should be back one indent level.
> + }
^
This pair of braces is not needed.
> + case RTS51X_STAT_SS:{
> + US_DEBUGP("%s: RTS51X_STAT_SS, intf->pm_usage_cnt:%d,"
> + "power.usage:%d\n", __func__,
> + atomic_read(&us->pusb_intf->pm_usage_cnt),
> + atomic_read(&us->pusb_intf->dev.power.usage_count));
> +
> + if (atomic_read(&us->pusb_intf->pm_usage_cnt) > 0) {
> + US_DEBUGP("%s: Ready to enter SS state.\n",
> + __func__);
> + rts51x_set_stat(chip, RTS51X_STAT_SS);
> + /* ignore mass storage interface's children */
> + pm_suspend_ignore_children(&us->pusb_intf->dev, true);
> + usb_autopm_put_interface(us->pusb_intf);
> + US_DEBUGP("%s: RTS51X_STAT_SS 01,"
> + "intf->pm_usage_cnt:%d, power.usage:%d\n",
> + __func__,
> + atomic_read(&us->pusb_intf->pm_usage_cnt),
> + atomic_read(
> + &us->pusb_intf->dev.power.usage_count));
> + }
> + break;
> + }
^
Same situation with these braces.
> + default:
> + US_DEBUGP("%s: Unknonwn state !!!\n", __func__);
> + break;
> + }
> +
> + if (rts51x_get_stat(chip) != RTS51X_STAT_SS)
> + queue_delayed_work(chip->rts51x_wq,
> + &(chip->rts51x_delayed_work),
^ ^
These parens are not needed.
> + POLLING_INTERVAL * HZ / 1000);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Use msecs_to_jiffies(POLLING_INTERVAL)
> + /* unlock the device pointers */
> + mutex_unlock(&us->dev_mutex);
> +
> + US_DEBUGP("%s: --->\n", __func__);
> +}
> +
regards,
dan carpenter
--
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/
Powered by blists - more mailing lists