Re: [PATCH 15/39] acpi/battery: simplify procfs code
On Thu, Apr 19, 2018 at 2:41 PM, Christoph Hellwig wrote: > Use remove_proc_subtree to remove the whole subtree on cleanup, and > unwind the registration loop into individual calls. Switch to use > proc_create_seq where applicable. > > Signed-off-by: Christoph Hellwig It is OK AFAICS. Reviewed-by: Rafael J. Wysocki
Re: [PATCH 2/2] Fix a race condition between SPI domain validation and system suspend
On Monday, January 8, 2018 5:02:53 PM CET Martin K. Petersen wrote: > > Bart, > > > Avoid that the following warning is reported when suspending a system > > that is using the mptspi driver: > > Acked-by: Martin K. Petersen Thanks! Let me take both patches to the linux-pm tree then. Thanks, Rafael
Re: [PATCH 1/2] PM / sleep: Make lock/unlock_system_sleep() available to kernel modules
On Sat, Jan 6, 2018 at 12:32 AM, Bart Van Assche wrote: > On Sat, 2018-01-06 at 00:00 +0100, Rafael J. Wysocki wrote: >> The changes are fine by me, but I really would prefer them to go in >> via the PM tree which I guess means that the second patch would need >> to go in this way too. > > Hello Rafael, > > If I can get a Tested-by from Woody and an Acked-by from Martin then I can > do that. That would be an ACK from Marting for the second patch. OK
Re: [PATCH 1/2] PM / sleep: Make lock/unlock_system_sleep() available to kernel modules
On Fri, Jan 5, 2018 at 6:19 PM, Bart Van Assche wrote:
> Since pm_mutex is not exported using lock/unlock_system_sleep() from
> inside a kernel module causes a "pm_mutex undefined" linker error.
> Hence move lock/unlock_system_sleep() into kernel/power/main.c and
> export these.
>
> Signed-off-by: Bart Van Assche
> Cc: Rafael J. Wysocki
> ---
> include/linux/suspend.h | 28 ++--
> kernel/power/main.c | 29 +
> 2 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index d60b0f5c38d5..cc22a24516d6 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -443,32 +443,8 @@ extern bool pm_save_wakeup_count(unsigned int count);
> extern void pm_wakep_autosleep_enabled(bool set);
> extern void pm_print_active_wakeup_sources(void);
>
> -static inline void lock_system_sleep(void)
> -{
> - current->flags |= PF_FREEZER_SKIP;
> - mutex_lock(&pm_mutex);
> -}
> -
> -static inline void unlock_system_sleep(void)
> -{
> - /*
> -* Don't use freezer_count() because we don't want the call to
> -* try_to_freeze() here.
> -*
> -* Reason:
> -* Fundamentally, we just don't need it, because freezing condition
> -* doesn't come into effect until we release the pm_mutex lock,
> -* since the freezer always works with pm_mutex held.
> -*
> -* More importantly, in the case of hibernation,
> -* unlock_system_sleep() gets called in snapshot_read() and
> -* snapshot_write() when the freezing condition is still in effect.
> -* Which means, if we use try_to_freeze() here, it would make them
> -* enter the refrigerator, thus causing hibernation to lockup.
> -*/
> - current->flags &= ~PF_FREEZER_SKIP;
> - mutex_unlock(&pm_mutex);
> -}
> +extern void lock_system_sleep(void);
> +extern void unlock_system_sleep(void);
>
> #else /* !CONFIG_PM_SLEEP */
Don't you need to add stubs for this case too?
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 3a2ca9066583..705c2366dafe 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -22,6 +22,35 @@ DEFINE_MUTEX(pm_mutex);
>
> #ifdef CONFIG_PM_SLEEP
>
> +void lock_system_sleep(void)
> +{
> + current->flags |= PF_FREEZER_SKIP;
> + mutex_lock(&pm_mutex);
> +}
> +EXPORT_SYMBOL_GPL(lock_system_sleep);
> +
> +void unlock_system_sleep(void)
> +{
> + /*
> +* Don't use freezer_count() because we don't want the call to
> +* try_to_freeze() here.
> +*
> +* Reason:
> +* Fundamentally, we just don't need it, because freezing condition
> +* doesn't come into effect until we release the pm_mutex lock,
> +* since the freezer always works with pm_mutex held.
> +*
> +* More importantly, in the case of hibernation,
> +* unlock_system_sleep() gets called in snapshot_read() and
> +* snapshot_write() when the freezing condition is still in effect.
> +* Which means, if we use try_to_freeze() here, it would make them
> +* enter the refrigerator, thus causing hibernation to lockup.
> +*/
> + current->flags &= ~PF_FREEZER_SKIP;
> + mutex_unlock(&pm_mutex);
> +}
> +EXPORT_SYMBOL_GPL(unlock_system_sleep);
> +
> /* Routines for PM-transition notifications */
>
> static BLOCKING_NOTIFIER_HEAD(pm_chain_head);
> --
The changes are fine by me, but I really would prefer them to go in
via the PM tree which I guess means that the second patch would need
to go in this way too.
Thanks,
Rafael
Re: [trivial PATCH] treewide: Align function definition open/close braces
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> @@ -2449,8 +2449,7 @@ xfs_agf_verify(
>be32_to_cpu(agf->agf_refcount_level) > XFS_BTREE_MAXLEVELS))
> return false;
>
> - return true;;
> -
> + return true;
> }
>
> static void
> diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
> index fe1bfee35898..7d5c355d78b5 100644
> --- a/fs/xfs/xfs_export.c
> +++ b/fs/xfs/xfs_export.c
> @@ -122,7 +122,7 @@ xfs_nfs_get_inode(
> struct super_block *sb,
> u64 ino,
> u32 generation)
> - {
> +{
> xfs_mount_t *mp = XFS_M(sb);
> xfs_inode_t *ip;
> int error;
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 227db99b0f19..d97e8f0f73ca 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -443,15 +443,15 @@ static int audit_set_failure(u32 state)
> * Drop any references inside the auditd connection tracking struct and free
> * the memory.
> */
> - static void auditd_conn_free(struct rcu_head *rcu)
> - {
> +static void auditd_conn_free(struct rcu_head *rcu)
> +{
> struct auditd_connection *ac;
>
> ac = container_of(rcu, struct auditd_connection, rcu);
> put_pid(ac->pid);
> put_net(ac->net);
> kfree(ac);
> - }
> +}
>
> /**
> * auditd_set - Set/Reset the auditd connection state
> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index ad1d6164e946..50f44b7b2b32 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -196,7 +196,7 @@ struct notifier_block module_trace_bprintk_format_nb = {
> };
>
> int __trace_bprintk(unsigned long ip, const char *fmt, ...)
> - {
> +{
> int ret;
> va_list ap;
>
> @@ -214,7 +214,7 @@ int __trace_bprintk(unsigned long ip, const char *fmt,
> ...)
> EXPORT_SYMBOL_GPL(__trace_bprintk);
>
> int __ftrace_vbprintk(unsigned long ip, const char *fmt, va_list ap)
> - {
> +{
> if (unlikely(!fmt))
> return 0;
>
> diff --git a/lib/raid6/sse2.c b/lib/raid6/sse2.c
> index 1d2276b007ee..8191e1d0d2fb 100644
> --- a/lib/raid6/sse2.c
> +++ b/lib/raid6/sse2.c
> @@ -91,7 +91,7 @@ static void raid6_sse21_gen_syndrome(int disks, size_t
> bytes, void **ptrs)
>
> static void raid6_sse21_xor_syndrome(int disks, int start, int stop,
>size_t bytes, void **ptrs)
> - {
> +{
> u8 **dptr = (u8 **)ptrs;
> u8 *p, *q;
> int d, z, z0;
> @@ -200,9 +200,9 @@ static void raid6_sse22_gen_syndrome(int disks, size_t
> bytes, void **ptrs)
> kernel_fpu_end();
> }
>
> - static void raid6_sse22_xor_syndrome(int disks, int start, int stop,
> +static void raid6_sse22_xor_syndrome(int disks, int start, int stop,
>size_t bytes, void **ptrs)
> - {
> +{
> u8 **dptr = (u8 **)ptrs;
> u8 *p, *q;
> int d, z, z0;
> @@ -265,7 +265,7 @@ static void raid6_sse22_gen_syndrome(int disks, size_t
> bytes, void **ptrs)
>
> asm volatile("sfence" : : : "memory");
> kernel_fpu_end();
> - }
> +}
>
> const struct raid6_calls raid6_sse2x2 = {
> raid6_sse22_gen_syndrome,
> @@ -366,9 +366,9 @@ static void raid6_sse24_gen_syndrome(int disks, size_t
> bytes, void **ptrs)
> kernel_fpu_end();
> }
>
> - static void raid6_sse24_xor_syndrome(int disks, int start, int stop,
> +static void raid6_sse24_xor_syndrome(int disks, int start, int stop,
>size_t bytes, void **ptrs)
> - {
> +{
> u8 **dptr = (u8 **)ptrs;
> u8 *p, *q;
> int d, z, z0;
> @@ -471,7 +471,7 @@ static void raid6_sse24_gen_syndrome(int disks, size_t
> bytes, void **ptrs)
> }
> asm volatile("sfence" : : : "memory");
> kernel_fpu_end();
> - }
> +}
>
>
> const struct raid6_calls raid6_sse2x4 = {
> diff --git a/sound/soc/fsl/fsl_dma.c b/sound/soc/fsl/fsl_dma.c
> index 0c11f434a374..ec619f51d336 100644
> --- a/sound/soc/fsl/fsl_dma.c
> +++ b/sound/soc/fsl/fsl_dma.c
> @@ -879,7 +879,7 @@ static const struct snd_pcm_ops fsl_dma_ops = {
> };
>
> static int fsl_soc_dma_probe(struct platform_device *pdev)
> - {
> +{
> struct dma_object *dma;
> struct device_node *np = pdev->dev.of_node;
> struct device_node *ssi_np;
>
> --
Acked-by: Rafael J. Wysocki
for the ACPI part.
Thanks!
Re: [PATCH v2 09/15] ACPI: configfs: make config_item_type const
On Monday, October 16, 2017 5:18:48 PM CEST Bhumika Goyal wrote:
> Make these structures const as they are either passed to the functions
> having the argument as const or stored as a reference in the "ci_type"
> const field of a config_item structure.
>
> Done using Coccienlle.
>
> Signed-off-by: Bhumika Goyal
> ---
> * Changes in v2- Combine all the followup patches and the constification
> patches into a series.
>
> drivers/acpi/acpi_configfs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/acpi_configfs.c b/drivers/acpi/acpi_configfs.c
> index 853bc7f..b588503 100644
> --- a/drivers/acpi/acpi_configfs.c
> +++ b/drivers/acpi/acpi_configfs.c
> @@ -204,7 +204,7 @@ struct configfs_attribute *acpi_table_attrs[] = {
> NULL,
> };
>
> -static struct config_item_type acpi_table_type = {
> +static const struct config_item_type acpi_table_type = {
> .ct_owner = THIS_MODULE,
> .ct_bin_attrs = acpi_table_bin_attrs,
> .ct_attrs = acpi_table_attrs,
> @@ -237,12 +237,12 @@ struct configfs_group_operations acpi_table_group_ops =
> {
> .drop_item = acpi_table_drop_item,
> };
>
> -static struct config_item_type acpi_tables_type = {
> +static const struct config_item_type acpi_tables_type = {
> .ct_owner = THIS_MODULE,
> .ct_group_ops = &acpi_table_group_ops,
> };
>
> -static struct config_item_type acpi_root_group_type = {
> +static const struct config_item_type acpi_root_group_type = {
> .ct_owner = THIS_MODULE,
> };
>
>
Acked-by: Rafael J. Wysocki
Re: [PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn
On Fri, Feb 17, 2017 at 8:11 AM, Joe Perches wrote: > There are ~4300 uses of pr_warn and ~250 uses of the older > pr_warning in the kernel source tree. > > Make the use of pr_warn consistent across all kernel files. > > This excludes all files in tools/ as there is a separate > define pr_warning for that directory tree and pr_warn is > not used in tools/. > > Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing. Sorry about asking if that has been asked already. Wouldn't it be slightly less intrusive to simply redefined pr_warning() as a synonym for pr_warn()? Thanks, Rafael
Re: [PATCH 1/4] pnp: pnpbios: Add explicit X86_32 dependency to PNPBIOS
On 4/11/2016 3:25 PM, William Breathitt Gray wrote: The PNPBIOS driver requires preprocessor defines (located in include/asm/segment.h) only declared if the architecture is set to X86_32. If the architecture is set to X86_64, the PNPBIOS driver will not build properly. The X86 dependecy for the PNPBIOS configuration option is changed to an explicit X86_32 dependency in order to prevent an attempt to build for an unsupported architecture. Cc: Rafael J. Wysocki Signed-off-by: William Breathitt Gray Has anyone taken care of this already? If not, can you possibly resend this patch with a CC to linux-a...@vger.kernel.org so I can pick it up via Patchwork more easily? --- drivers/pnp/pnpbios/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pnp/pnpbios/Kconfig b/drivers/pnp/pnpbios/Kconfig index 50c3dd0..a786086 100644 --- a/drivers/pnp/pnpbios/Kconfig +++ b/drivers/pnp/pnpbios/Kconfig @@ -3,7 +3,7 @@ # config PNPBIOS bool "Plug and Play BIOS support" - depends on ISA && X86 + depends on ISA && X86_32 default n ---help--- Linux uses the PNPBIOS as defined in "Plug and Play BIOS -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] PM / sleep: try to runtime suspend for direct complete
On Wednesday, February 24, 2016 04:22:27 PM Derek Basehore wrote:
> This tries to runtime suspend devices that are still active for direct
> complete. This is for cases such as autosuspend delays which leaves
> devices able to runtime suspend but still active. It's beneficial in
> this case to runtime suspend the device to take advantage of direct
> complete when possible.
>
> Signed-off-by: Derek Basehore
> Reviewed-by: Eric Caruso
> ---
> drivers/base/power/main.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index e0017d9..9693032 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1380,7 +1380,12 @@ static int __device_suspend(struct device *dev,
> pm_message_t state, bool async)
> goto Complete;
>
> if (dev->power.direct_complete) {
> - if (pm_runtime_status_suspended(dev)) {
> + /*
> + * Check if we're runtime suspended. If not, try to runtime
> + * suspend for autosuspend cases.
> + */
> + if (pm_runtime_status_suspended(dev) ||
> + !pm_runtime_suspend(dev)) {
> pm_runtime_disable(dev);
> if (pm_runtime_status_suspended(dev))
> goto Complete;
>
This can be done somewhat simpler, because pm_runtime_suspend() will check
if the device has already been suspended:
ret = pm_runtime_suspend(dev);
if (ret >= 0) {
and you don't need the extra pm_runtime_status_suspended(dev) check.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Monday, September 28, 2015 10:24:58 AM Arnd Bergmann wrote: > On Sunday 27 September 2015 16:10:48 Rafael J. Wysocki wrote: > > On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote: > > > On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > > > > On 25 September 2015 at 15:19, Rafael J. Wysocki > > > > wrote: > > > > > So if you allow something like debugfs to update your structure, how > > > > > do you make sure there is the proper locking? > > > > > > > > Not really sure at all.. Isn't there some debugfs locking that will > > > > jump in, to avoid updation of fields to the same device? > > > > > > No, if you need any locking to access variable, you cannot use the > > > simple debugfs helpers but have to provide your own functions. > > > > > > > >> Anyway, that problem isn't here for sure as its between two > > > > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > > > > > > > I guess it might be more convenient to fold this into the other patch, > > > > > because we seem to be splitting hairs here. > > > > > > > > I can and that's what I did. But then Arnd asked me to separate it > > > > out. I can fold it back if that's what you want. > > > > > > It still makes sense to keep it separate I think, the patch is clearly > > > different from the other parts. > > > > I just don't see much point in going from unsigned long to u32 and then > > from 32 to bool if we can go directly to bool in one go. > > It's only important to keep the 34-file multi-subsystem trivial cleanup > that doesn't change any functionality separate from the bugfix. Which isn't a bugfix really, because the EC code is not run on any big-endian systems to my knowledge. And it won't matter after the [2/2] anyway. And the changelog of it doesn't really makes sense, because it talks about future systems, but after the [2/2] no future systems will run that code in the first place. > If you like to avoid patching one of the files twice, the alternative would > be to first change the API for all other instances from u32 to bool > and leave ACPI alone, and then do the second patch that changes ACPI > from long to bool. My point is that this patch doesn't matter. It doesn't really fix anything and the result of it goes away after the second patch. The only marginal value of having it as a separate commit is in case if (a) we need to revert the [2/2] for some reason and (b) ACPI-based ARM systems (the big-endian ones) become full-hardware at one point. You know what the chances of that are, though. :-) That said I've ACKed the patch, because I don't care that much. I'm not exactly sure why you care either. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Sun, Sep 27, 2015 at 12:04 AM, Viresh Kumar wrote:
> global_lock is defined as an unsigned long and accessing only its lower
> 32 bits from sysfs is incorrect, as we need to consider other 32 bits
> for big endian 64-bit systems. There are no such platforms yet, but the
> code needs to be robust for such a case.
>
> Fix that by changing type of 'global_lock' to u32.
>
> Signed-off-by: Viresh Kumar
Acked-by: Rafael J. Wysocki
Greg, please take this one along with the [2/2] if that one looks good to you.
> ---
> BCC'd a lot of people (rather than cc'ing them) to make sure
> - the series reaches them
> - mailing lists do not block the patchset due to long cc list
> - and we don't spam the BCC'd people for every reply
>
> V4->V5:
> - Switch back to the original solution of making global_lock u32.
> ---
> drivers/acpi/ec_sys.c | 2 +-
> drivers/acpi/internal.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
> index b4c216bab22b..bea8e425a8de 100644
> --- a/drivers/acpi/ec_sys.c
> +++ b/drivers/acpi/ec_sys.c
> @@ -128,7 +128,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
> unsigned int ec_device_count)
> if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
> goto error;
> if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
> -(u32 *)&first_ec->global_lock))
> +&first_ec->global_lock))
> goto error;
>
> if (write_support)
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 9e426210c2a8..9db196de003c 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -138,7 +138,7 @@ struct acpi_ec {
> unsigned long gpe;
> unsigned long command_addr;
> unsigned long data_addr;
> - unsigned long global_lock;
> + u32 global_lock;
> unsigned long flags;
> unsigned long reference_count;
> struct mutex mutex;
> --
> 2.4.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote: > On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > > On 25 September 2015 at 15:19, Rafael J. Wysocki wrote: > > > So if you allow something like debugfs to update your structure, how > > > do you make sure there is the proper locking? > > > > Not really sure at all.. Isn't there some debugfs locking that will > > jump in, to avoid updation of fields to the same device? > > No, if you need any locking to access variable, you cannot use the > simple debugfs helpers but have to provide your own functions. > > > >> Anyway, that problem isn't here for sure as its between two > > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > > > I guess it might be more convenient to fold this into the other patch, > > > because we seem to be splitting hairs here. > > > > I can and that's what I did. But then Arnd asked me to separate it > > out. I can fold it back if that's what you want. > > It still makes sense to keep it separate I think, the patch is clearly > different from the other parts. I just don't see much point in going from unsigned long to u32 and then from 32 to bool if we can go directly to bool in one go. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Saturday, September 26, 2015 12:52:08 PM James Bottomley wrote: > On Fri, 2015-09-25 at 22:58 +0200, Rafael J. Wysocki wrote: > > On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote: > > > On 25 September 2015 at 13:33, Rafael J. Wysocki > > > wrote: > > > > You're going to change that into bool in the next patch, right? > > > > > > Yeah. > > > > > > > So what if bool is a byte and the field is not word-aligned > > > > > > Its between two 'unsigned long' variables today, and the struct isn't > > > packed. > > > So, it will be aligned, isn't it? > > > > > > > and changing > > > > that byte requires a read-modify-write. How do we ensure that things > > > > remain > > > > consistent in that case? > > > > > > I didn't understood why a read-modify-write is special here? That's > > > what will happen > > > to most of the non-word-sized fields anyway? > > > > > > Probably I didn't understood what you meant.. > > > > Say you have three adjacent fields in a structure, x, y, z, each one byte > > long. > > Initially, all of them are equal to 0. > > > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > > > What's the result? > > I think every CPU's cache architecure guarantees adjacent store > integrity, even in the face of SMP, so it's x==1 and y==2. If you're > thinking of old alpha SMP system where the lowest store width is 32 bits > and thus you have to do RMW to update a byte, this was usually fixed by > padding (assuming the structure is not packed). However, it was such a > problem that even the later alpha chips had byte extensions. OK, thanks! Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Fri, Sep 25, 2015 at 11:44 PM, Viresh Kumar wrote: > On 25-09-15, 22:58, Rafael J. Wysocki wrote: >> Say you have three adjacent fields in a structure, x, y, z, each one byte >> long. >> Initially, all of them are equal to 0. >> >> CPU A writes 1 to x and CPU B writes 2 to y at the same time. >> >> What's the result? > > But then two CPUs can update the same variable as well, and we must > have proper locking in place to fix that. Right. So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking? Is that not a problem in all of the places modified by the [2/2]? How can the future users of the API know what to do to avoid potential problems with it? > > Anyway, that problem isn't here for sure as its between two > unsigned-longs. So, should I just move it to bool and resend ? I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote: > On 25 September 2015 at 13:33, Rafael J. Wysocki wrote: > > You're going to change that into bool in the next patch, right? > > Yeah. > > > So what if bool is a byte and the field is not word-aligned > > Its between two 'unsigned long' variables today, and the struct isn't packed. > So, it will be aligned, isn't it? > > > and changing > > that byte requires a read-modify-write. How do we ensure that things remain > > consistent in that case? > > I didn't understood why a read-modify-write is special here? That's > what will happen > to most of the non-word-sized fields anyway? > > Probably I didn't understood what you meant.. Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0. CPU A writes 1 to x and CPU B writes 2 to y at the same time. What's the result? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 10:26:22 PM Rafael J. Wysocki wrote: > On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote: > > On 25-09-15, 20:49, Johannes Berg wrote: > > > Ok, then, but that means Rafael is completely wrong ... > > > debugfs_create_bool() takes a *pointer* and it needs to be long-lived, > > > it can't be on the stack. You also don't get a call when it changes. > > > > Ahh, ofcourse. My bad as well... > > Well, sorry about the wrong suggestion. > > > I think we can change structure definition but will wait for Rafael's > > comment before that. > > OK, change the structure then. But here's a question. You're going to change that into bool in the next patch, right? So what if bool is a byte and the field is not word-aligned and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote: > On 25-09-15, 20:49, Johannes Berg wrote: > > Ok, then, but that means Rafael is completely wrong ... > > debugfs_create_bool() takes a *pointer* and it needs to be long-lived, > > it can't be on the stack. You also don't get a call when it changes. > > Ahh, ofcourse. My bad as well... Well, sorry about the wrong suggestion. > I think we can change structure definition but will wait for Rafael's > comment before that. OK, change the structure then. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 10:18:13 PM Rafael J. Wysocki wrote: > On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote: > > global_lock is defined as an unsigned long and accessing only its lower > > 32 bits from sysfs is incorrect, as we need to consider other 32 bits > > for big endian 64 bit systems. There are no such platforms yet, but the > > code needs to be robust for such a case. > > > > Fix that by passing a local variable to debugfs_create_bool() and > > assigning its value to global_lock later. > > > > Signed-off-by: Viresh Kumar > > Acked-by: Rafael J. Wysocki > > Greg, please take this one if the [2/2] looks good to you. Ouch, turns out it was a bad idea. Please scratch that. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote:
> global_lock is defined as an unsigned long and accessing only its lower
> 32 bits from sysfs is incorrect, as we need to consider other 32 bits
> for big endian 64 bit systems. There are no such platforms yet, but the
> code needs to be robust for such a case.
>
> Fix that by passing a local variable to debugfs_create_bool() and
> assigning its value to global_lock later.
>
> Signed-off-by: Viresh Kumar
Acked-by: Rafael J. Wysocki
Greg, please take this one if the [2/2] looks good to you.
> ---
> V3->V4:
> - Create a local variable instead of changing type of global_lock
> (Rafael)
> - Drop the stable tag
> - BCC'd a lot of people (rather than cc'ing them) to make sure
> - the series reaches them
> - mailing lists do not block the patchset due to long cc list
> - and we don't spam the BCC'd people for every reply
> ---
> drivers/acpi/ec_sys.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
> index b4c216bab22b..b44b91331a56 100644
> --- a/drivers/acpi/ec_sys.c
> +++ b/drivers/acpi/ec_sys.c
> @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
> unsigned int ec_device_count)
> struct dentry *dev_dir;
> char name[64];
> umode_t mode = 0400;
> + u32 val;
>
> if (ec_device_count == 0) {
> acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
> @@ -127,10 +128,11 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
> unsigned int ec_device_count)
>
> if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
> goto error;
> - if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
> - (u32 *)&first_ec->global_lock))
> + if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val))
> goto error;
>
> + first_ec->global_lock = val;
> +
> if (write_support)
> mode = 0600;
> if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
>
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Update][PATCH] SCSI / PM: Replace CONFIG_PM_RUNTIME with CONFIG_PM
From: Rafael J. Wysocki
Subject: SCSI / PM: Replace CONFIG_PM_RUNTIME with CONFIG_PM
After commit b2b49ccbdd54 (PM: Kconfig: Set PM_RUNTIME if PM_SLEEP is
selected) PM_RUNTIME is always set if PM is set, so #ifdef blocks
depending on CONFIG_PM_RUNTIME may now be changed to depend on
CONFIG_PM.
Replace CONFIG_PM_RUNTIME with CONFIG_PM everywhere under
drivers/scsi/ and in include/scsi/scsi_device.h.
Signed-off-by: Rafael J. Wysocki
---
Note: This depends on commit b2b49ccbdd54 (PM: Kconfig: Set PM_RUNTIME if
PM_SLEEP is selected) which is only in linux-next at the moment (via the
linux-pm tree).
Please let me know if it is OK to take this one into linux-pm.
---
drivers/scsi/scsi_pm.c | 10 --
drivers/scsi/scsi_priv.h |5 ++---
drivers/scsi/ufs/ufshcd-pci.c| 11 ---
drivers/scsi/ufs/ufshcd-pltfrm.c | 11 ---
include/scsi/scsi_device.h |4 ++--
5 files changed, 12 insertions(+), 29 deletions(-)
Index: linux-pm/include/scsi/scsi_device.h
===
--- linux-pm.orig/include/scsi/scsi_device.h
+++ linux-pm/include/scsi/scsi_device.h
@@ -440,13 +440,13 @@ static inline int scsi_execute_req(struc
extern void sdev_disable_disk_events(struct scsi_device *sdev);
extern void sdev_enable_disk_events(struct scsi_device *sdev);
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
extern int scsi_autopm_get_device(struct scsi_device *);
extern void scsi_autopm_put_device(struct scsi_device *);
#else
static inline int scsi_autopm_get_device(struct scsi_device *d) { return 0; }
static inline void scsi_autopm_put_device(struct scsi_device *d) {}
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
static inline int __must_check scsi_device_reprobe(struct scsi_device *sdev)
{
Index: linux-pm/drivers/scsi/scsi_priv.h
===
--- linux-pm.orig/drivers/scsi/scsi_priv.h
+++ linux-pm/drivers/scsi/scsi_priv.h
@@ -157,8 +157,7 @@ static inline void scsi_netlink_exit(voi
/* scsi_pm.c */
#ifdef CONFIG_PM
extern const struct dev_pm_ops scsi_bus_pm_ops;
-#endif
-#ifdef CONFIG_PM_RUNTIME
+
extern void scsi_autopm_get_target(struct scsi_target *);
extern void scsi_autopm_put_target(struct scsi_target *);
extern int scsi_autopm_get_host(struct Scsi_Host *);
@@ -168,7 +167,7 @@ static inline void scsi_autopm_get_targe
static inline void scsi_autopm_put_target(struct scsi_target *t) {}
static inline int scsi_autopm_get_host(struct Scsi_Host *h) { return 0; }
static inline void scsi_autopm_put_host(struct Scsi_Host *h) {}
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
extern struct async_domain scsi_sd_pm_domain;
extern struct async_domain scsi_sd_probe_domain;
Index: linux-pm/drivers/scsi/scsi_pm.c
===
--- linux-pm.orig/drivers/scsi/scsi_pm.c
+++ linux-pm/drivers/scsi/scsi_pm.c
@@ -213,8 +213,6 @@ static int scsi_bus_restore(struct devic
#endif /* CONFIG_PM_SLEEP */
-#ifdef CONFIG_PM_RUNTIME
-
static int sdev_runtime_suspend(struct device *dev)
{
const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
@@ -332,14 +330,6 @@ void scsi_autopm_put_host(struct Scsi_Ho
pm_runtime_put_sync(&shost->shost_gendev);
}
-#else
-
-#define scsi_runtime_suspend NULL
-#define scsi_runtime_resumeNULL
-#define scsi_runtime_idle NULL
-
-#endif /* CONFIG_PM_RUNTIME */
-
const struct dev_pm_ops scsi_bus_pm_ops = {
.prepare = scsi_bus_prepare,
.suspend = scsi_bus_suspend,
Index: linux-pm/drivers/scsi/ufs/ufshcd-pci.c
===
--- linux-pm.orig/drivers/scsi/ufs/ufshcd-pci.c
+++ linux-pm/drivers/scsi/ufs/ufshcd-pci.c
@@ -62,12 +62,7 @@ static int ufshcd_pci_resume(struct devi
{
return ufshcd_system_resume(dev_get_drvdata(dev));
}
-#else
-#define ufshcd_pci_suspend NULL
-#define ufshcd_pci_resume NULL
-#endif /* CONFIG_PM */
-#ifdef CONFIG_PM_RUNTIME
static int ufshcd_pci_runtime_suspend(struct device *dev)
{
return ufshcd_runtime_suspend(dev_get_drvdata(dev));
@@ -80,11 +75,13 @@ static int ufshcd_pci_runtime_idle(struc
{
return ufshcd_runtime_idle(dev_get_drvdata(dev));
}
-#else /* !CONFIG_PM_RUNTIME */
+#else /* !CONFIG_PM */
+#define ufshcd_pci_suspend NULL
+#define ufshcd_pci_resume NULL
#define ufshcd_pci_runtime_suspend NULL
#define ufshcd_pci_runtime_resume NULL
#define ufshcd_pci_runtime_idleNULL
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
/**
* ufshcd_pci_shutdown - main function to put the controller in reset state
Index: linux-pm/drivers/scsi/ufs/ufshcd-pltfrm.c
===
--- linux-pm.orig/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ linux-pm/drivers/scsi/ufs/ufshcd-plt
Re: [PATCH] SCSI / PM: Replace CONFIG_PM_RUNTIME with CONFIG_PM
On Tuesday, December 09, 2014 02:19:57 PM Aaron Lu wrote:
> On 12/04/2014 09:22 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki
> >
> > After commit b2b49ccbdd54 (PM: Kconfig: Set PM_RUNTIME if PM_SLEEP is
> > selected) PM_RUNTIME is always set if PM is set, so #ifdef blocks
> > depending on CONFIG_PM_RUNTIME may now be changed to depend on
> > CONFIG_PM.
> >
> > Replace CONFIG_PM_RUNTIME with CONFIG_PM everywhere under
> > drivers/scsi/ and in include/scsi/scsi_device.h.
> >
> > Signed-off-by: Rafael J. Wysocki
> > ---
> >
> > Note: This depends on commit b2b49ccbdd54 (PM: Kconfig: Set PM_RUNTIME if
> > PM_SLEEP is selected) which is only in linux-next at the moment (via the
> > linux-pm tree).
> >
> > Please let me know if it is OK to take this one into linux-pm.
> >
> > ---
> > drivers/scsi/scsi_pm.c |4 ++--
> > drivers/scsi/scsi_priv.h |5 ++---
> > drivers/scsi/ufs/ufshcd-pci.c| 11 ---
> > drivers/scsi/ufs/ufshcd-pltfrm.c | 11 ---
> > include/scsi/scsi_device.h |4 ++--
> > 5 files changed, 14 insertions(+), 21 deletions(-)
> >
> > Index: linux-pm/drivers/scsi/scsi_pm.c
> > ===
> > --- linux-pm.orig/drivers/scsi/scsi_pm.c
> > +++ linux-pm/drivers/scsi/scsi_pm.c
> > @@ -213,7 +213,7 @@ static int scsi_bus_restore(struct devic
> >
> > #endif /* CONFIG_PM_SLEEP */
> >
> > -#ifdef CONFIG_PM_RUNTIME
> > +#ifdef CONFIG_PM
> >
> > static int sdev_runtime_suspend(struct device *dev)
> > {
> > @@ -338,7 +338,7 @@ void scsi_autopm_put_host(struct Scsi_Ho
> > #define scsi_runtime_resumeNULL
> > #define scsi_runtime_idle NULL
> >
> > -#endif /* CONFIG_PM_RUNTIME */
> > +#endif /* CONFIG_PM */
> >
> > const struct dev_pm_ops scsi_bus_pm_ops = {
> > .prepare = scsi_bus_prepare,
>
> Just one nitpeak: since scsi_pm.o is only compiled when CONFIG_PM is
> set, I think we can skip the #ifdef CONFIG_PM ... #endif macro in this
> file now. I mean something like this:
Ah, right, I overlooked that, thanks!
I've folded the below into the SCSI patch.
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 7454498..9e43ae1 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -213,8 +213,6 @@ static int scsi_bus_restore(struct device *dev)
>
> #endif /* CONFIG_PM_SLEEP */
>
> -#ifdef CONFIG_PM_RUNTIME
> -
> static int sdev_runtime_suspend(struct device *dev)
> {
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> @@ -332,14 +330,6 @@ void scsi_autopm_put_host(struct Scsi_Host *shost)
> pm_runtime_put_sync(&shost->shost_gendev);
> }
>
> -#else
> -
> -#define scsi_runtime_suspend NULL
> -#define scsi_runtime_resume NULL
> -#define scsi_runtime_idleNULL
> -
> -#endif /* CONFIG_PM_RUNTIME */
> -
> const struct dev_pm_ops scsi_bus_pm_ops = {
> .prepare = scsi_bus_prepare,
> .suspend = scsi_bus_suspend,
>
> Thanks,
> Aaron
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] SCSI / PM: Replace CONFIG_PM_RUNTIME with CONFIG_PM
From: Rafael J. Wysocki
After commit b2b49ccbdd54 (PM: Kconfig: Set PM_RUNTIME if PM_SLEEP is
selected) PM_RUNTIME is always set if PM is set, so #ifdef blocks
depending on CONFIG_PM_RUNTIME may now be changed to depend on
CONFIG_PM.
Replace CONFIG_PM_RUNTIME with CONFIG_PM everywhere under
drivers/scsi/ and in include/scsi/scsi_device.h.
Signed-off-by: Rafael J. Wysocki
---
Note: This depends on commit b2b49ccbdd54 (PM: Kconfig: Set PM_RUNTIME if
PM_SLEEP is selected) which is only in linux-next at the moment (via the
linux-pm tree).
Please let me know if it is OK to take this one into linux-pm.
---
drivers/scsi/scsi_pm.c |4 ++--
drivers/scsi/scsi_priv.h |5 ++---
drivers/scsi/ufs/ufshcd-pci.c| 11 ---
drivers/scsi/ufs/ufshcd-pltfrm.c | 11 ---
include/scsi/scsi_device.h |4 ++--
5 files changed, 14 insertions(+), 21 deletions(-)
Index: linux-pm/include/scsi/scsi_device.h
===
--- linux-pm.orig/include/scsi/scsi_device.h
+++ linux-pm/include/scsi/scsi_device.h
@@ -440,13 +440,13 @@ static inline int scsi_execute_req(struc
extern void sdev_disable_disk_events(struct scsi_device *sdev);
extern void sdev_enable_disk_events(struct scsi_device *sdev);
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
extern int scsi_autopm_get_device(struct scsi_device *);
extern void scsi_autopm_put_device(struct scsi_device *);
#else
static inline int scsi_autopm_get_device(struct scsi_device *d) { return 0; }
static inline void scsi_autopm_put_device(struct scsi_device *d) {}
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
static inline int __must_check scsi_device_reprobe(struct scsi_device *sdev)
{
Index: linux-pm/drivers/scsi/scsi_priv.h
===
--- linux-pm.orig/drivers/scsi/scsi_priv.h
+++ linux-pm/drivers/scsi/scsi_priv.h
@@ -157,8 +157,7 @@ static inline void scsi_netlink_exit(voi
/* scsi_pm.c */
#ifdef CONFIG_PM
extern const struct dev_pm_ops scsi_bus_pm_ops;
-#endif
-#ifdef CONFIG_PM_RUNTIME
+
extern void scsi_autopm_get_target(struct scsi_target *);
extern void scsi_autopm_put_target(struct scsi_target *);
extern int scsi_autopm_get_host(struct Scsi_Host *);
@@ -168,7 +167,7 @@ static inline void scsi_autopm_get_targe
static inline void scsi_autopm_put_target(struct scsi_target *t) {}
static inline int scsi_autopm_get_host(struct Scsi_Host *h) { return 0; }
static inline void scsi_autopm_put_host(struct Scsi_Host *h) {}
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
extern struct async_domain scsi_sd_pm_domain;
extern struct async_domain scsi_sd_probe_domain;
Index: linux-pm/drivers/scsi/scsi_pm.c
===
--- linux-pm.orig/drivers/scsi/scsi_pm.c
+++ linux-pm/drivers/scsi/scsi_pm.c
@@ -213,7 +213,7 @@ static int scsi_bus_restore(struct devic
#endif /* CONFIG_PM_SLEEP */
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
static int sdev_runtime_suspend(struct device *dev)
{
@@ -338,7 +338,7 @@ void scsi_autopm_put_host(struct Scsi_Ho
#define scsi_runtime_resumeNULL
#define scsi_runtime_idle NULL
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
const struct dev_pm_ops scsi_bus_pm_ops = {
.prepare = scsi_bus_prepare,
Index: linux-pm/drivers/scsi/ufs/ufshcd-pci.c
===
--- linux-pm.orig/drivers/scsi/ufs/ufshcd-pci.c
+++ linux-pm/drivers/scsi/ufs/ufshcd-pci.c
@@ -62,12 +62,7 @@ static int ufshcd_pci_resume(struct devi
{
return ufshcd_system_resume(dev_get_drvdata(dev));
}
-#else
-#define ufshcd_pci_suspend NULL
-#define ufshcd_pci_resume NULL
-#endif /* CONFIG_PM */
-#ifdef CONFIG_PM_RUNTIME
static int ufshcd_pci_runtime_suspend(struct device *dev)
{
return ufshcd_runtime_suspend(dev_get_drvdata(dev));
@@ -80,11 +75,13 @@ static int ufshcd_pci_runtime_idle(struc
{
return ufshcd_runtime_idle(dev_get_drvdata(dev));
}
-#else /* !CONFIG_PM_RUNTIME */
+#else /* !CONFIG_PM */
+#define ufshcd_pci_suspend NULL
+#define ufshcd_pci_resume NULL
#define ufshcd_pci_runtime_suspend NULL
#define ufshcd_pci_runtime_resume NULL
#define ufshcd_pci_runtime_idleNULL
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
/**
* ufshcd_pci_shutdown - main function to put the controller in reset state
Index: linux-pm/drivers/scsi/ufs/ufshcd-pltfrm.c
===
--- linux-pm.orig/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ linux-pm/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -266,12 +266,7 @@ static int ufshcd_pltfrm_resume(struct d
{
return ufshcd_system_resume(dev_get_drvdata(dev));
}
-#else
-#define ufshcd_pltfrm_suspend NULL
-#define ufshcd_pltfrm_resume NULL
-#endif
-#ifdef CONFIG_PM_RUNTIME
static int
[Update][PATCH 8/9] powerpc / eeh_driver: Use global PCI rescan-remove locking
From: Rafael J. Wysocki
Subject: powerpc / eeh_driver: Use global PCI rescan-remove locking
Race conditions are theoretically possible between the PCI device
addition and removal in the PPC64 PCI error recovery driver and
the generic PCI bus rescan and device removal that can be triggered
via sysfs.
To avoid those race conditions make PPC64 PCI error recovery driver
use global PCI rescan-remove locking around PCI device addition and
removal.
Signed-off-by: Rafael J. Wysocki
---
The previous version had wrong function names in the last hunk, sorry about
that.
Rafael
---
arch/powerpc/kernel/eeh_driver.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
Index: linux-pm/arch/powerpc/kernel/eeh_driver.c
===
--- linux-pm.orig/arch/powerpc/kernel/eeh_driver.c
+++ linux-pm/arch/powerpc/kernel/eeh_driver.c
@@ -369,7 +369,9 @@ static void *eeh_rmv_device(void *data,
edev->mode |= EEH_DEV_DISCONNECTED;
(*removed)++;
+ pci_lock_rescan_remove();
pci_stop_and_remove_bus_device(dev);
+ pci_unlock_rescan_remove();
return NULL;
}
@@ -416,10 +418,13 @@ static int eeh_reset_device(struct eeh_p
* into pcibios_add_pci_devices().
*/
eeh_pe_state_mark(pe, EEH_PE_KEEP);
- if (bus)
+ if (bus) {
+ pci_lock_rescan_remove();
pcibios_remove_pci_devices(bus);
- else if (frozen_bus)
+ pci_unlock_rescan_remove();
+ } else if (frozen_bus) {
eeh_pe_dev_traverse(pe, eeh_rmv_device, &removed);
+ }
/* Reset the pci controller. (Asserts RST#; resets config space).
* Reconfigure bridges and devices. Don't try to bring the system
@@ -429,6 +434,8 @@ static int eeh_reset_device(struct eeh_p
if (rc)
return rc;
+ pci_lock_rescan_remove();
+
/* Restore PE */
eeh_ops->configure_bridge(pe);
eeh_pe_restore_bars(pe);
@@ -462,6 +469,7 @@ static int eeh_reset_device(struct eeh_p
pe->tstamp = tstamp;
pe->freeze_count = cnt;
+ pci_unlock_rescan_remove();
return 0;
}
@@ -618,8 +626,11 @@ perm_error:
eeh_pe_dev_traverse(pe, eeh_report_failure, NULL);
/* Shut down the device drivers for good. */
- if (frozen_bus)
+ if (frozen_bus) {
+ pci_lock_rescan_remove();
pcibios_remove_pci_devices(frozen_bus);
+ pci_unlock_rescan_remove();
+ }
}
static void eeh_handle_special_event(void)
@@ -692,6 +703,7 @@ static void eeh_handle_special_event(voi
if (rc == 2 || rc == 1)
eeh_handle_normal_event(pe);
else {
+ pci_lock_rescan_remove();
list_for_each_entry_safe(hose, tmp,
&hose_list, list_node) {
phb_pe = eeh_phb_pe_get(hose);
@@ -703,6 +715,7 @@ static void eeh_handle_special_event(voi
eeh_pe_dev_traverse(pe, eeh_report_failure, NULL);
pcibios_remove_pci_devices(bus);
}
+ pci_unlock_rescan_remove();
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/9] ACPI / hotplug / PCI: Use global PCI rescan-remove locking
From: Rafael J. Wysocki
Multiple race conditions are possible between the ACPI-based PCI
hotplug (ACPIPHP) and the generic PCI bus rescan and device removal
that can be triggered via sysfs.
To avoid those race conditions make the ACPIPHP code use global PCI
rescan-remove locking.
Signed-off-by: Rafael J. Wysocki
---
drivers/pci/hotplug/acpiphp.h |5 +++-
drivers/pci/hotplug/acpiphp_core.c |2 -
drivers/pci/hotplug/acpiphp_glue.c | 43 -
3 files changed, 43 insertions(+), 7 deletions(-)
Index: linux-pm/drivers/pci/hotplug/acpiphp.h
===
--- linux-pm.orig/drivers/pci/hotplug/acpiphp.h
+++ linux-pm/drivers/pci/hotplug/acpiphp.h
@@ -77,6 +77,8 @@ struct acpiphp_bridge {
/* PCI-to-PCI bridge device */
struct pci_dev *pci_dev;
+
+ bool is_going_away;
};
@@ -150,6 +152,7 @@ struct acpiphp_attention_info
/* slot flags */
#define SLOT_ENABLED (0x0001)
+#define SLOT_IS_GOING_AWAY (0x0002)
/* function flags */
@@ -169,7 +172,7 @@ void acpiphp_unregister_hotplug_slot(str
typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
int acpiphp_enable_slot(struct acpiphp_slot *slot);
-int acpiphp_disable_and_eject_slot(struct acpiphp_slot *slot);
+int acpiphp_disable_slot(struct acpiphp_slot *slot);
u8 acpiphp_get_power_status(struct acpiphp_slot *slot);
u8 acpiphp_get_attention_status(struct acpiphp_slot *slot);
u8 acpiphp_get_latch_status(struct acpiphp_slot *slot);
Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -432,6 +432,7 @@ static void cleanup_bridge(struct acpiph
pr_err("failed to remove notify
handler\n");
}
}
+ slot->flags |= SLOT_IS_GOING_AWAY;
if (slot->slot)
acpiphp_unregister_hotplug_slot(slot);
}
@@ -439,6 +440,8 @@ static void cleanup_bridge(struct acpiph
mutex_lock(&bridge_mutex);
list_del(&bridge->list);
mutex_unlock(&bridge_mutex);
+
+ bridge->is_going_away = true;
}
/**
@@ -757,6 +760,10 @@ static void acpiphp_check_bridge(struct
{
struct acpiphp_slot *slot;
+ /* Bail out if the bridge is going away. */
+ if (bridge->is_going_away)
+ return;
+
list_for_each_entry(slot, &bridge->slots, node) {
struct pci_bus *bus = slot->bus;
struct pci_dev *dev, *tmp;
@@ -827,6 +834,8 @@ void acpiphp_check_host_bridge(acpi_hand
}
}
+static int acpiphp_disable_and_eject_slot(struct acpiphp_slot *slot);
+
static void hotplug_event(acpi_handle handle, u32 type, void *data)
{
struct acpiphp_context *context = data;
@@ -856,6 +865,9 @@ static void hotplug_event(acpi_handle ha
} else {
struct acpiphp_slot *slot = func->slot;
+ if (slot->flags & SLOT_IS_GOING_AWAY)
+ break;
+
mutex_lock(&slot->crit_sect);
enable_slot(slot);
mutex_unlock(&slot->crit_sect);
@@ -871,6 +883,9 @@ static void hotplug_event(acpi_handle ha
struct acpiphp_slot *slot = func->slot;
int ret;
+ if (slot->flags & SLOT_IS_GOING_AWAY)
+ break;
+
/*
* Check if anything has changed in the slot and rescan
* from the parent if that's the case.
@@ -900,9 +915,11 @@ static void hotplug_event_work(void *dat
acpi_handle handle = context->handle;
acpi_scan_lock_acquire();
+ pci_lock_rescan_remove();
hotplug_event(handle, type, context);
+ pci_unlock_rescan_remove();
acpi_scan_lock_release();
acpi_evaluate_hotplug_ost(handle, type, ACPI_OST_SC_SUCCESS, NULL);
put_bridge(context->func.parent);
@@ -1070,12 +1087,19 @@ void acpiphp_remove_slots(struct pci_bus
*/
int acpiphp_enable_slot(struct acpiphp_slot *slot)
{
+ pci_lock_rescan_remove();
+
+ if (slot->flags & SLOT_IS_GOING_AWAY)
+ return -ENODEV;
+
mutex_lock(&slot->crit_sect);
/* configure all functions */
if (!(slot->flags & SLOT_ENABLED))
enable_slot(slot);
mutex_unlock(&slot->crit_sect);
+
+ pci_unlock_rescan_remove();
return 0;
}
@@ -1083,10 +1107,12 @@ int acpiphp_enable_slot(struct acpiphp_s
* acpiphp_disable_and_eject_slot - power off
[PATCH 2/9] ACPI / PCI: Use global PCI rescan-remove locking in PCI root hotplug
From: Rafael J. Wysocki
Multiple race conditions are possible between the addition and
removal of PCI devices during ACPI PCI host bridge hotplug and the
generic PCI bus rescan and device removal that can be triggered via
sysfs.
To avoid those race conditions make the ACPI PCI host bridge addition
and removal code use global PCI rescan-remove locking.
Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/pci_root.c |6 ++
1 file changed, 6 insertions(+)
Index: linux-pm/drivers/acpi/pci_root.c
===
--- linux-pm.orig/drivers/acpi/pci_root.c
+++ linux-pm/drivers/acpi/pci_root.c
@@ -604,7 +604,9 @@ static int acpi_pci_root_add(struct acpi
pci_assign_unassigned_root_bus_resources(root->bus);
}
+ pci_lock_rescan_remove();
pci_bus_add_devices(root->bus);
+ pci_unlock_rescan_remove();
return 1;
end:
@@ -616,6 +618,8 @@ static void acpi_pci_root_remove(struct
{
struct acpi_pci_root *root = acpi_driver_data(device);
+ pci_lock_rescan_remove();
+
pci_stop_root_bus(root->bus);
device_set_run_wake(root->bus->bridge, false);
@@ -623,6 +627,8 @@ static void acpi_pci_root_remove(struct
pci_remove_root_bus(root->bus);
+ pci_unlock_rescan_remove();
+
kfree(root);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/9] PCI / hotplug: Use global PCI rescan-remove locking
From: Rafael J. Wysocki
Multiple race conditions are possible between PCI hotplug and the
generic PCI bus rescan and device removal that can be triggered via
sysfs.
To avoid those race conditions make PCI hotplug use global PCI
rescan-remove locking.
Signed-off-by: Rafael J. Wysocki
---
drivers/pci/hotplug/cpci_hotplug_pci.c | 14 --
drivers/pci/hotplug/cpqphp_pci.c |8 +++-
drivers/pci/hotplug/ibmphp_core.c | 13 +++--
drivers/pci/hotplug/pciehp_pci.c | 17 +
drivers/pci/hotplug/rpadlpar_core.c| 19 ++-
drivers/pci/hotplug/rpaphp_core.c |4
drivers/pci/hotplug/s390_pci_hpc.c |4 +++-
drivers/pci/hotplug/sgi_hotplug.c |5 +
drivers/pci/hotplug/shpchp_pci.c | 18 ++
9 files changed, 83 insertions(+), 19 deletions(-)
Index: linux-pm/drivers/pci/hotplug/rpadlpar_core.c
===
--- linux-pm.orig/drivers/pci/hotplug/rpadlpar_core.c
+++ linux-pm/drivers/pci/hotplug/rpadlpar_core.c
@@ -354,10 +354,15 @@ int dlpar_remove_pci_slot(char *drc_name
{
struct pci_bus *bus;
struct slot *slot;
+ int ret = 0;
+
+ pci_lock_rescan_remove();
bus = pcibios_find_pci_bus(dn);
- if (!bus)
- return -EINVAL;
+ if (!bus) {
+ ret = -EINVAL;
+ goto out;
+ }
pr_debug("PCI: Removing PCI slot below EADS bridge %s\n",
bus->self ? pci_name(bus->self) : "");
@@ -371,7 +376,8 @@ int dlpar_remove_pci_slot(char *drc_name
printk(KERN_ERR
"%s: unable to remove hotplug slot %s\n",
__func__, drc_name);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
}
@@ -382,7 +388,8 @@ int dlpar_remove_pci_slot(char *drc_name
if (pcibios_unmap_io_space(bus)) {
printk(KERN_ERR "%s: failed to unmap bus range\n",
__func__);
- return -ERANGE;
+ ret = -ERANGE;
+ goto out;
}
/* Remove the EADS bridge device itself */
@@ -390,7 +397,9 @@ int dlpar_remove_pci_slot(char *drc_name
pr_debug("PCI: Now removing bridge device %s\n", pci_name(bus->self));
pci_stop_and_remove_bus_device(bus->self);
- return 0;
+ out:
+ pci_unlock_rescan_remove();
+ return ret;
}
/**
Index: linux-pm/drivers/pci/hotplug/cpci_hotplug_pci.c
===
--- linux-pm.orig/drivers/pci/hotplug/cpci_hotplug_pci.c
+++ linux-pm/drivers/pci/hotplug/cpci_hotplug_pci.c
@@ -254,9 +254,12 @@ int __ref cpci_configure_slot(struct slo
{
struct pci_dev *dev;
struct pci_bus *parent;
+ int ret = 0;
dbg("%s - enter", __func__);
+ pci_lock_rescan_remove();
+
if (slot->dev == NULL) {
dbg("pci_dev null, finding %02x:%02x:%x",
slot->bus->number, PCI_SLOT(slot->devfn),
PCI_FUNC(slot->devfn));
@@ -277,7 +280,8 @@ int __ref cpci_configure_slot(struct slo
slot->dev = pci_get_slot(slot->bus, slot->devfn);
if (slot->dev == NULL) {
err("Could not find PCI device for slot %02x",
slot->number);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out;
}
}
parent = slot->dev->bus;
@@ -294,8 +298,10 @@ int __ref cpci_configure_slot(struct slo
pci_bus_add_devices(parent);
+ out:
+ pci_unlock_rescan_remove();
dbg("%s - exit", __func__);
- return 0;
+ return ret;
}
int cpci_unconfigure_slot(struct slot* slot)
@@ -308,6 +314,8 @@ int cpci_unconfigure_slot(struct slot* s
return -ENODEV;
}
+ pci_lock_rescan_remove();
+
list_for_each_entry_safe(dev, temp, &slot->bus->devices, bus_list) {
if (PCI_SLOT(dev->devfn) != PCI_SLOT(slot->devfn))
continue;
@@ -318,6 +326,8 @@ int cpci_unconfigure_slot(struct slot* s
pci_dev_put(slot->dev);
slot->dev = NULL;
+ pci_unlock_rescan_remove();
+
dbg("%s - exit", __func__);
return 0;
}
Index: linux-pm/drivers/pci/hotplug/pciehp_pci.c
===
--- linux-pm.orig/drivers/pci/hotplug/pciehp_pci.c
+++ linux-pm/drivers/pci/hotplug/pciehp_pci.c
@@ -39,22 +39,26 @@ int pciehp_configure_device(struct slot
struct pci_dev *dev;
struct pci_dev *bridge =
[PATCH 0/9] PCI: Eliminate race conditions between hotplug and sysfs rescan/remove (Was: Re: [PATCH v2 04/10] PCI: Destroy pci dev only once)
[Cc: adding linux-scsi for the MPT changes, Ben for powerpc, Matthew for platform/x86 and Konrad for Xen] On Friday, December 06, 2013 02:21:50 AM Rafael J. Wysocki wrote: [...] > > OK > > To be a bit more constructive, as the next step I'd try to use > pci_remove_rescan_mutex to serialize all PCI hotplug operations (as I said > above) without making the other changes made by my patch. Does that sound > reasonable? Well, no answer here, so as a followup, a series implementing that idea follows. I *hope* I found all of the places that need to be synchronized vs the bus rescan and device removal that can be triggered via sysfs, but I might overlook something. Also in some cases I wasn't quite sure how much stuff to put under the lock, because said stuff is not exactly straightforward. Enjoy! Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 8/9] powerpc / eeh_driver: Use global PCI rescan-remove locking
From: Rafael J. Wysocki
Race conditions are theoretically possible between the PCI device
addition and removal in the PPC64 PCI error recovery driver and
the generic PCI bus rescan and device removal that can be triggered
via sysfs.
To avoid those race conditions make PPC64 PCI error recovery driver
use global PCI rescan-remove locking around PCI device addition and
removal.
Signed-off-by: Rafael J. Wysocki
---
arch/powerpc/kernel/eeh_driver.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
Index: linux-pm/arch/powerpc/kernel/eeh_driver.c
===
--- linux-pm.orig/arch/powerpc/kernel/eeh_driver.c
+++ linux-pm/arch/powerpc/kernel/eeh_driver.c
@@ -369,7 +369,9 @@ static void *eeh_rmv_device(void *data,
edev->mode |= EEH_DEV_DISCONNECTED;
(*removed)++;
+ pci_lock_rescan_remove();
pci_stop_and_remove_bus_device(dev);
+ pci_unlock_rescan_remove();
return NULL;
}
@@ -416,10 +418,13 @@ static int eeh_reset_device(struct eeh_p
* into pcibios_add_pci_devices().
*/
eeh_pe_state_mark(pe, EEH_PE_KEEP);
- if (bus)
+ if (bus) {
+ pci_lock_rescan_remove();
pcibios_remove_pci_devices(bus);
- else if (frozen_bus)
+ pci_unlock_rescan_remove();
+ } else if (frozen_bus) {
eeh_pe_dev_traverse(pe, eeh_rmv_device, &removed);
+ }
/* Reset the pci controller. (Asserts RST#; resets config space).
* Reconfigure bridges and devices. Don't try to bring the system
@@ -429,6 +434,8 @@ static int eeh_reset_device(struct eeh_p
if (rc)
return rc;
+ pci_lock_rescan_remove();
+
/* Restore PE */
eeh_ops->configure_bridge(pe);
eeh_pe_restore_bars(pe);
@@ -462,6 +469,7 @@ static int eeh_reset_device(struct eeh_p
pe->tstamp = tstamp;
pe->freeze_count = cnt;
+ pci_unlock_rescan_remove();
return 0;
}
@@ -618,8 +626,11 @@ perm_error:
eeh_pe_dev_traverse(pe, eeh_report_failure, NULL);
/* Shut down the device drivers for good. */
- if (frozen_bus)
+ if (frozen_bus) {
+ pci_lock_rescan_remove();
pcibios_remove_pci_devices(frozen_bus);
+ pci_unlock_rescan_remove();
+ }
}
static void eeh_handle_special_event(void)
@@ -692,6 +703,7 @@ static void eeh_handle_special_event(voi
if (rc == 2 || rc == 1)
eeh_handle_normal_event(pe);
else {
+ lock_pci_remove_rescan();
list_for_each_entry_safe(hose, tmp,
&hose_list, list_node) {
phb_pe = eeh_phb_pe_get(hose);
@@ -703,6 +715,7 @@ static void eeh_handle_special_event(voi
eeh_pe_dev_traverse(pe, eeh_report_failure, NULL);
pcibios_remove_pci_devices(bus);
}
+ unlock_pci_remove_rescan();
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/9] PCMCIA / cardbus: Use global PCI rescan-remove locking
From: Rafael J. Wysocki Multiple race conditions are possible between the cardbus PCI device addition and removal and the generic PCI bus rescan and device removal that can be triggered via sysfs. To avoid those race conditions make the cardbus code use global PCI rescan-remove locking. Signed-off-by: Rafael J. Wysocki --- drivers/pcmcia/cardbus.c |7 +++ 1 file changed, 7 insertions(+) Index: linux-pm/drivers/pcmcia/cardbus.c === --- linux-pm.orig/drivers/pcmcia/cardbus.c +++ linux-pm/drivers/pcmcia/cardbus.c @@ -70,6 +70,8 @@ int __ref cb_alloc(struct pcmcia_socket struct pci_dev *dev; unsigned int max, pass; + pci_lock_rescan_remove(); + s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0)); pci_fixup_cardbus(bus); @@ -93,6 +95,7 @@ int __ref cb_alloc(struct pcmcia_socket pci_bus_add_devices(bus); + pci_unlock_rescan_remove(); return 0; } @@ -115,6 +118,10 @@ void cb_free(struct pcmcia_socket *s) if (!bus) return; + pci_lock_rescan_remove(); + list_for_each_entry_safe(dev, tmp, &bus->devices, bus_list) pci_stop_and_remove_bus_device(dev); + + pci_unlock_rescan_remove(); } -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/9] platform / x86: Use global PCI rescan-remove locking
From: Rafael J. Wysocki
Multiple race conditions are possible between the rfkill hotplug in
the asus-wmi and eeepc-laptop drivers and the generic PCI bus rescan
and device removal that can be triggered via sysfs.
To avoid those race conditions make asus-wmi and eeepc-laptop use
global PCI rescan-remove locking around the rfkill hotplug.
Signed-off-by: Rafael J. Wysocki
---
drivers/platform/x86/asus-wmi.c |2 ++
drivers/platform/x86/eeepc-laptop.c |2 ++
2 files changed, 4 insertions(+)
Index: linux-pm/drivers/platform/x86/asus-wmi.c
===
--- linux-pm.orig/drivers/platform/x86/asus-wmi.c
+++ linux-pm/drivers/platform/x86/asus-wmi.c
@@ -605,6 +605,7 @@ static void asus_rfkill_hotplug(struct a
mutex_unlock(&asus->wmi_lock);
mutex_lock(&asus->hotplug_lock);
+ pci_lock_rescan_remove();
if (asus->wlan.rfkill)
rfkill_set_sw_state(asus->wlan.rfkill, blocked);
@@ -655,6 +656,7 @@ static void asus_rfkill_hotplug(struct a
}
out_unlock:
+ pci_unlock_rescan_remove();
mutex_unlock(&asus->hotplug_lock);
}
Index: linux-pm/drivers/platform/x86/eeepc-laptop.c
===
--- linux-pm.orig/drivers/platform/x86/eeepc-laptop.c
+++ linux-pm/drivers/platform/x86/eeepc-laptop.c
@@ -591,6 +591,7 @@ static void eeepc_rfkill_hotplug(struct
rfkill_set_sw_state(eeepc->wlan_rfkill, blocked);
mutex_lock(&eeepc->hotplug_lock);
+ pci_lock_rescan_remove();
if (eeepc->hotplug_slot) {
port = acpi_get_pci_dev(handle);
@@ -648,6 +649,7 @@ out_put_dev:
}
out_unlock:
+ pci_unlock_rescan_remove();
mutex_unlock(&eeepc->hotplug_lock);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/9] PCI: Global rescan-remove lock
From: Rafael J. Wysocki
There are multiple PCI device addition and removal code paths that
may be run concurrently with the generic PCI bus rescan and device
removal that can be triggered via sysfs. If that happens, it may
lead to multiple different, potentially dangerous race conditions.
The most straightforward way to address those problems is to run
the code in question under the same lock that is used by the
generic rescan/remove code in pci-sysfs.c. To prepare for those
changes, move the definition of the global PCI remove/rescan lock
to probe.c and provide global wrappers, pci_lock_rescan_remove()
and pci_unlock_rescan_remove(), allowing drivers to manipulate
that lock. Also provide pci_stop_and_remove_bus_device_locked()
for the callers of pci_stop_and_remove_bus_device() who only need
to hold the rescan/remove lock around it.
Signed-off-by: Rafael J. Wysocki
---
drivers/pci/pci-sysfs.c | 19 +++
drivers/pci/probe.c | 18 ++
drivers/pci/remove.c|8
include/linux/pci.h |3 +++
4 files changed, 36 insertions(+), 12 deletions(-)
Index: linux-pm/drivers/pci/pci-sysfs.c
===
--- linux-pm.orig/drivers/pci/pci-sysfs.c
+++ linux-pm/drivers/pci/pci-sysfs.c
@@ -297,7 +297,6 @@ msi_bus_store(struct device *dev, struct
}
static DEVICE_ATTR_RW(msi_bus);
-static DEFINE_MUTEX(pci_remove_rescan_mutex);
static ssize_t bus_rescan_store(struct bus_type *bus, const char *buf,
size_t count)
{
@@ -308,10 +307,10 @@ static ssize_t bus_rescan_store(struct b
return -EINVAL;
if (val) {
- mutex_lock(&pci_remove_rescan_mutex);
+ pci_lock_rescan_remove();
while ((b = pci_find_next_bus(b)) != NULL)
pci_rescan_bus(b);
- mutex_unlock(&pci_remove_rescan_mutex);
+ pci_unlock_rescan_remove();
}
return count;
}
@@ -342,9 +341,9 @@ dev_rescan_store(struct device *dev, str
return -EINVAL;
if (val) {
- mutex_lock(&pci_remove_rescan_mutex);
+ pci_lock_rescan_remove();
pci_rescan_bus(pdev->bus);
- mutex_unlock(&pci_remove_rescan_mutex);
+ pci_unlock_rescan_remove();
}
return count;
}
@@ -354,11 +353,7 @@ static struct device_attribute dev_resca
static void remove_callback(struct device *dev)
{
- struct pci_dev *pdev = to_pci_dev(dev);
-
- mutex_lock(&pci_remove_rescan_mutex);
- pci_stop_and_remove_bus_device(pdev);
- mutex_unlock(&pci_remove_rescan_mutex);
+ pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
}
static ssize_t
@@ -395,12 +390,12 @@ dev_bus_rescan_store(struct device *dev,
return -EINVAL;
if (val) {
- mutex_lock(&pci_remove_rescan_mutex);
+ pci_lock_rescan_remove();
if (!pci_is_root_bus(bus) && list_empty(&bus->devices))
pci_rescan_bus_bridge_resize(bus->self);
else
pci_rescan_bus(bus);
- mutex_unlock(&pci_remove_rescan_mutex);
+ pci_unlock_rescan_remove();
}
return count;
}
Index: linux-pm/drivers/pci/probe.c
===
--- linux-pm.orig/drivers/pci/probe.c
+++ linux-pm/drivers/pci/probe.c
@@ -2014,6 +2014,24 @@ EXPORT_SYMBOL(pci_scan_slot);
EXPORT_SYMBOL(pci_scan_bridge);
EXPORT_SYMBOL_GPL(pci_scan_child_bus);
+/*
+ * pci_rescan_bus(), pci_rescan_bus_bridge_resize() and PCI device removal
+ * routines should always be executed under this mutex.
+ */
+static DEFINE_MUTEX(pci_rescan_remove_lock);
+
+void pci_lock_rescan_remove(void)
+{
+ mutex_lock(&pci_rescan_remove_lock);
+}
+EXPORT_SYMBOL_GPL(pci_lock_rescan_remove);
+
+void pci_unlock_rescan_remove(void)
+{
+ mutex_unlock(&pci_rescan_remove_lock);
+}
+EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove);
+
static int __init pci_sort_bf_cmp(const struct device *d_a, const struct
device *d_b)
{
const struct pci_dev *a = to_pci_dev(d_a);
Index: linux-pm/include/linux/pci.h
===
--- linux-pm.orig/include/linux/pci.h
+++ linux-pm/include/linux/pci.h
@@ -779,6 +779,7 @@ struct pci_dev *pci_dev_get(struct pci_d
void pci_dev_put(struct pci_dev *dev);
void pci_remove_bus(struct pci_bus *b);
void pci_stop_and_remove_bus_device(struct pci_dev *dev);
+void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev);
void pci_stop_root_bus(struct pci_bus *bus);
void pci_remove_root_bus(struct pci_bus *bus);
void pci_setup_cardbus(struct pci_bus *bus);
@@ -1022,6 +1023,8 @@ void set_pcie_hotplug_bridge(struct pci_
int pci_bus_find_capability
[PATCH 9/9] Xen / PCI: Use global PCI rescan-remove locking
From: Rafael J. Wysocki
Multiple race conditions are possible between the Xen pcifront
device addition and removal and the generic PCI device addition
and removal that can be triggered via sysfs.
To avoid those race conditions make the Xen pcifront code use global
PCI rescan-remove locking.
Signed-off-by: Rafael J. Wysocki
---
drivers/pci/xen-pcifront.c |8
1 file changed, 8 insertions(+)
Index: linux-pm/drivers/pci/xen-pcifront.c
===
--- linux-pm.orig/drivers/pci/xen-pcifront.c
+++ linux-pm/drivers/pci/xen-pcifront.c
@@ -471,12 +471,15 @@ static int pcifront_scan_root(struct pci
}
pcifront_init_sd(sd, domain, bus, pdev);
+ pci_lock_rescan_remove();
+
b = pci_scan_bus_parented(&pdev->xdev->dev, bus,
&pcifront_bus_ops, sd);
if (!b) {
dev_err(&pdev->xdev->dev,
"Error creating PCI Frontend Bus!\n");
err = -ENOMEM;
+ pci_unlock_rescan_remove();
goto err_out;
}
@@ -494,6 +497,7 @@ static int pcifront_scan_root(struct pci
/* Create SysFS and notify udev of the devices. Aka: "going live" */
pci_bus_add_devices(b);
+ pci_unlock_rescan_remove();
return err;
err_out:
@@ -556,6 +560,7 @@ static void pcifront_free_roots(struct p
dev_dbg(&pdev->xdev->dev, "cleaning up root buses\n");
+ pci_lock_rescan_remove();
list_for_each_entry_safe(bus_entry, t, &pdev->root_buses, list) {
list_del(&bus_entry->list);
@@ -568,6 +573,7 @@ static void pcifront_free_roots(struct p
kfree(bus_entry);
}
+ pci_unlock_rescan_remove();
}
static pci_ers_result_t pcifront_common_process(int cmd,
@@ -1043,8 +1049,10 @@ static int pcifront_detach_devices(struc
domain, bus, slot, func);
continue;
}
+ pci_lock_rescan_remove();
pci_stop_and_remove_bus_device(pci_dev);
pci_dev_put(pci_dev);
+ pci_unlock_rescan_remove();
dev_dbg(&pdev->xdev->dev,
"PCI device %04x:%02x:%02x.%d removed.\n",
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/9] MPT / PCI: Use pci_stop_and_remove_bus_device_locked()
From: Rafael J. Wysocki Race conditions are theoretically possible between the MPT PCI device removal and the generic PCI bus rescan and device removal that can be triggered via sysfs. To avoid those race conditions make the MPT PCI code use pci_stop_and_remove_bus_device_locked(). Signed-off-by: Rafael J. Wysocki --- drivers/message/fusion/mptbase.c|2 +- drivers/scsi/mpt2sas/mpt2sas_base.c |2 +- drivers/scsi/mpt3sas/mpt3sas_base.c |2 +- 3 files changed, 3 insertions(+), 3 deletions(-) Index: linux-pm/drivers/scsi/mpt3sas/mpt3sas_base.c === --- linux-pm.orig/drivers/scsi/mpt3sas/mpt3sas_base.c +++ linux-pm/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -131,7 +131,7 @@ static int mpt3sas_remove_dead_ioc_func( pdev = ioc->pdev; if ((pdev == NULL)) return -1; - pci_stop_and_remove_bus_device(pdev); + pci_stop_and_remove_bus_device_locked(pdev); return 0; } Index: linux-pm/drivers/scsi/mpt2sas/mpt2sas_base.c === --- linux-pm.orig/drivers/scsi/mpt2sas/mpt2sas_base.c +++ linux-pm/drivers/scsi/mpt2sas/mpt2sas_base.c @@ -128,7 +128,7 @@ static int mpt2sas_remove_dead_ioc_func( pdev = ioc->pdev; if ((pdev == NULL)) return -1; - pci_stop_and_remove_bus_device(pdev); + pci_stop_and_remove_bus_device_locked(pdev); return 0; } Index: linux-pm/drivers/message/fusion/mptbase.c === --- linux-pm.orig/drivers/message/fusion/mptbase.c +++ linux-pm/drivers/message/fusion/mptbase.c @@ -346,7 +346,7 @@ static int mpt_remove_dead_ioc_func(void if ((pdev == NULL)) return -1; - pci_stop_and_remove_bus_device(pdev); + pci_stop_and_remove_bus_device_locked(pdev); return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: REQ_PM vs REQ_TYPE_PM_RESUME
On Thursday, January 09, 2014 01:17:12 PM Rafael J. Wysocki wrote: > On Thursday, January 09, 2014 09:29:59 AM Aaron Lu wrote: > > On 01/09/2014 05:21 AM, Alan Stern wrote: > > > On Wed, 8 Jan 2014, Phillip Susi wrote: > > >> You issue a REQUEST SENSE command and that returns status indicating > > >> whether the drive is stopped, or in standby. See my patches. One of > > > > > > I never saw your patches. Where were they posted? > > > > > > If you issue the REQUEST SENSE command in the usual way (going through > > > the SCSI and block layers), and the disk is already in runtime suspend, > > > it won't do what you want. The block layer won't deliver requests > > > until the device leaves the RPM_SUSPENDED state. In addition, when it > > > receives the command the block layer will submit a deferred > > > runtime-resume request, which rather defeats the whole purpose. > > > > > > (I guess you actually saw some of this happen, and that's what led to > > > this email thread in the first place.) > > > > > > It's a knotty situation. The only way to find out whether the disk is > > > spinning is to ask it, which requires doing I/O, which requires > > > spinning up the disk. Maybe we need to add a mechanism to the block > > > layer's runtime PM implementation for addressing just this situation. > > > > I think it's knotty because the runtime PM status is a view from the > > kernel/host side, i.e. it is runtime suspended if it is not being used, > > no matter which power state it is in. The trigger for the PM state > > transition are all based on this, if we want to do it the other way > > around(update device's runtime PM status based on device's actual power > > state), we are in a knotty situation. > > Agreed. That said during system resume, when we are trying to avoid resuming the device to save time/energy, it makes sense to check its physical state and do a pm_request_resume() if that is "on" to avoid a situation in which the device is physically "on", but its runtime PM status is "suspended" and it never gets powered down because of that. Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: REQ_PM vs REQ_TYPE_PM_RESUME
On Thursday, January 09, 2014 09:29:59 AM Aaron Lu wrote: > On 01/09/2014 05:21 AM, Alan Stern wrote: > > On Wed, 8 Jan 2014, Phillip Susi wrote: > >> You issue a REQUEST SENSE command and that returns status indicating > >> whether the drive is stopped, or in standby. See my patches. One of > > > > I never saw your patches. Where were they posted? > > > > If you issue the REQUEST SENSE command in the usual way (going through > > the SCSI and block layers), and the disk is already in runtime suspend, > > it won't do what you want. The block layer won't deliver requests > > until the device leaves the RPM_SUSPENDED state. In addition, when it > > receives the command the block layer will submit a deferred > > runtime-resume request, which rather defeats the whole purpose. > > > > (I guess you actually saw some of this happen, and that's what led to > > this email thread in the first place.) > > > > It's a knotty situation. The only way to find out whether the disk is > > spinning is to ask it, which requires doing I/O, which requires > > spinning up the disk. Maybe we need to add a mechanism to the block > > layer's runtime PM implementation for addressing just this situation. > > I think it's knotty because the runtime PM status is a view from the > kernel/host side, i.e. it is runtime suspended if it is not being used, > no matter which power state it is in. The trigger for the PM state > transition are all based on this, if we want to do it the other way > around(update device's runtime PM status based on device's actual power > state), we are in a knotty situation. Agreed. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] ata: acpi: rework the ata acpi bind support
On Friday, July 26, 2013 09:37:09 AM Aaron Lu wrote: > On 07/25/2013 10:52 PM, Tejun Heo wrote: > > On Thu, Jul 25, 2013 at 01:47:03PM +0800, Aaron Lu wrote: > >> Binding ACPI handle to SCSI device has several drawbacks, namely: > >> 1 During ATA device initialization time, ACPI handle will be needed > >> while SCSI devices are not created yet. So each time ACPI handle is > >> needed, instead of retrieving the handle by ACPI_HANDLE macro, > >> a namespace scan is performed to find the handle for the corresponding > >> ATA device. This is inefficient, and also expose a restriction on > >> calling path not holding any lock. > >> 2 The binding to SCSI device tree makes code complex, while at the same > >> time doesn't bring us any benefit. All ACPI handlings are still done > >> in ATA module, not in SCSI. > >> > >> Rework the ATA ACPI binding code to bind ACPI handle to ATA transport > >> devices(ATA port and ATA device). The binding needs to be done only once, > >> since the ATA transport devices do not go away with hotplug. And due to > >> this, the flush_work call in hotplug handler for ATA bay is no longer > >> needed. > > > > I like it but am wondering why we weren't doing this before. Was the > > acpi support added before we made ata objects proper devices? > > I think so. But since I didn't do the original binding, I can only guess > :-) > > At the time of the original binding, ACPI binding logic requires the > binding device has a bus type, which these ATA transport devices don't > have. > > Later, the ACPI binding code evolves and no such limitation exists > anymore. As you can see, we can simply set the ACPI handle before this > device is added in driver core, and the binding will be done. Yeah, we made that change in the ACPI core around 3.8 IIRC. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
On Friday, November 30, 2012 04:55:56 PM Aaron Lu wrote: > On 11/28/2012 09:39 AM, Tejun Heo wrote: > > Hey, Rafael. > > > > On Wed, Nov 28, 2012 at 01:51:00AM +0100, Rafael J. Wysocki wrote: > >> Having considered that a bit more I'm now thinking that in fact the power > >> state > >> the device is in at the moment doesn't really matter, so the polling code > >> need > >> not really know what PM is doing. What it needs to know is that the device > >> will generate a hardware event when something interesting happens, so it > >> is not > >> necessary to poll it. > >> > >> In this particular case it is related to power management (apparently, > >> hardware > >> events will only be generated after the device has been put into ACPI > >> D3cold, > >> or so Aaron seems to be claiming), but it need not be in general, at least > >> in > >> principle. > >> > >> It looks like we need an "event driven" flag that the can be set in the > >> lower > >> layers and read by the upper layers. I suppose this means it would need > >> to be > >> in struct device, but not necessarily in the PM-specific part of it. > > > > We already have that. That's what gendisk->async_events is for (as > > opposed to gendisk->events). If all events are async_events, block > > won't poll for events, but I'm not sure that's the golden bullet. > > > > * None implements async_events yet and an API is missing - > > disk_check_events() - which is trivial to add, but it's the same > > story. We'll need a mechanism to shoot up notification from libata > > to block layer. It's admittedly easier to justify routing through > > I don't see a way to do this, since libata has no chance of accessing > the gendisk pointer. Or we can add a new field to struct device, > something like no_poll, but I don't think it is the right thing to do, > as not all devices are block ones. > > Any other suggestions/ideas please? I think you can follow the James' suggestion: http://www.spinics.net/lists/linux-acpi/msg40257.html and see how that goes. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
On Monday, November 26, 2012 09:03:11 AM James Bottomley wrote: > On Mon, 2012-11-26 at 01:33 +0100, Rafael J. Wysocki wrote: > > On Monday, November 19, 2012 03:06:51 PM James Bottomley wrote: > > > > I really think we need a way for (auto)pm and event polling to talk to > > > > each other so that autopm can tell event poll to sod off while pm is > > > > in effect. Trying to solve this from inside libata doesn't seem > > > > right. The problem, again, seems to be figuring out which hardware > > > > device maps to which block device. Hmmm... Any good ideas? > > > > > > I've asked the PM people several times about this, because it's a real > > > problem for almost everything: PM needs some type of top to bottom > > > stack view, which the layering isolation we have within storage really > > > doesn't cope with well. No real suggestion has been forthcoming. > > > > Actually, I think that the particular case in question is really special > > and the fact that there's the pollig loop that user space is involved in > > doesn't make things more stratightforward. > > > > And PM really doesn't need to see things top to bottom, but the polling > > needs to know what happens in the PM land. We need to be able to tell it > > "from now on tell user space that there are no events here". The question > > is where to put that information so that it's accessible to all parts of the > > stack involved. > > Right, open to suggestions ... Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it. In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle. It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
On Monday, November 26, 2012 09:09:36 AM Aaron Lu wrote: > On 11/26/2012 09:11 AM, Rafael J. Wysocki wrote: > > On Monday, November 26, 2012 09:05:58 AM Aaron Lu wrote: > >> On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote: > >>> On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote: > >>>> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote: > >>>>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote: > >>>>>> On 11/20/2012 02:00 PM, Aaron Lu wrote: > >>>>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote: > >>>>>>>> I really think we need a way for (auto)pm and event polling to talk > >>>>>>>> to > >>>>>>>> each other so that autopm can tell event poll to sod off while pm is > >>>>>>>> in effect. Trying to solve this from inside libata doesn't seem > >>>>>>>> right. The problem, again, seems to be figuring out which hardware > >>>>>>>> device maps to which block device. Hmmm... Any good ideas? > >>>>>>> > >>>>>>> A possible way of doing this is using pm qos. > >>>>>>> > >>>>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and > >>>>>>> we > >>>>>>> can add another one: NO_POLL, use it like the following: > >>>>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is > >>>>>>> no > >>>>>>> longer necessary. In the ZPODD's case, it should be set when the > >>>>>>> device is to be powered off; > >>>>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when > >>>>>>> power > >>>>>>> is re-gained, this flag will be cleared. > >>>>>> > >>>>>> > >>>>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply > >>>>>>> return. > >>>>>> > >>>>>> It should be, skip calling disk->fops->check_events, but still queue > >>>>>> the > >>>>>> work for next time's poll. > >>>>>> > >>>>>> -Aaron > >>>>>> > >>>>>>> > >>>>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer > >>>>>>> can access it through ata_device->sdev->sdev_gendev. > >>>>>>> > >>>>>>> Is this OK? > >>>>> > >>>>> No, I don't think so. PM QoS is about telling the layer that will put > >>>>> the > >>>>> device into low-power states what states are to be taken into > >>>>> consideration. > >>>>> In this case, however, we need to tell someone else that the device has > >>>>> been > >>>>> turned off. Clearly, we need a way to do that, but not through PM QoS. > >>>>> > >>>>> Did you consider using pm_runtime_suspended() to check the device > >>>>> status? > >>>> > >>>> The problem is, a device can be in runtime suspended state while still > >>>> needs to be polled... > >>> > >>> Well, maybe this is the problem, then? Why does it need to be polled when > >>> suspended? > >> > >> For ODDs, poll is not necessary only when ZP capable ODD is powered off. > >> For other ODDs, poll still needs to go on. > >> > >> ZP capable ODDs: > >> - runtime suspended, power remained(due to NO_POWER_OFF qos flag) > >> poll is needed > >> -- runtime suspended, power removed > >> poll is not needed > >> > >> Non ZP capable ODDs: > >> -- runtime suspended, power remained (power will never be removed) > >> poll is needed > >> > >> If we do not poll for the powered on case, we will lose media change > >> event. > > > > But the media change event should change the status from suspended to > > active, > > shouldn't it? > > The media change event is derived from the poll, if we stop the poll, how > can we know the event in the first place? OK, so what you're trying to say is that if the device is not turned off and the user opens the tray and inserts a media in there, we won't know that that happened without polling. Is that correct? If so, can you please remind me why we want to pretend that the device is suspended if we want to poll it? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
On Monday, November 26, 2012 09:05:58 AM Aaron Lu wrote: > On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote: > > On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote: > >> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote: > >>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote: > >>>> On 11/20/2012 02:00 PM, Aaron Lu wrote: > >>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote: > >>>>>> I really think we need a way for (auto)pm and event polling to talk to > >>>>>> each other so that autopm can tell event poll to sod off while pm is > >>>>>> in effect. Trying to solve this from inside libata doesn't seem > >>>>>> right. The problem, again, seems to be figuring out which hardware > >>>>>> device maps to which block device. Hmmm... Any good ideas? > >>>>> > >>>>> A possible way of doing this is using pm qos. > >>>>> > >>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we > >>>>> can add another one: NO_POLL, use it like the following: > >>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no > >>>>> longer necessary. In the ZPODD's case, it should be set when the > >>>>> device is to be powered off; > >>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power > >>>>> is re-gained, this flag will be cleared. > >>>> > >>>> > >>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply > >>>>> return. > >>>> > >>>> It should be, skip calling disk->fops->check_events, but still queue the > >>>> work for next time's poll. > >>>> > >>>> -Aaron > >>>> > >>>>> > >>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer > >>>>> can access it through ata_device->sdev->sdev_gendev. > >>>>> > >>>>> Is this OK? > >>> > >>> No, I don't think so. PM QoS is about telling the layer that will put the > >>> device into low-power states what states are to be taken into > >>> consideration. > >>> In this case, however, we need to tell someone else that the device has > >>> been > >>> turned off. Clearly, we need a way to do that, but not through PM QoS. > >>> > >>> Did you consider using pm_runtime_suspended() to check the device status? > >> > >> The problem is, a device can be in runtime suspended state while still > >> needs to be polled... > > > > Well, maybe this is the problem, then? Why does it need to be polled when > > suspended? > > For ODDs, poll is not necessary only when ZP capable ODD is powered off. > For other ODDs, poll still needs to go on. > > ZP capable ODDs: > - runtime suspended, power remained(due to NO_POWER_OFF qos flag) > poll is needed > -- runtime suspended, power removed > poll is not needed > > Non ZP capable ODDs: > -- runtime suspended, power remained (power will never be removed) > poll is needed > > If we do not poll for the powered on case, we will lose media change > event. But the media change event should change the status from suspended to active, shouldn't it? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote: > On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote: > > On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote: > >> On 11/20/2012 02:00 PM, Aaron Lu wrote: > >>> On 11/19/2012 10:56 PM, Tejun Heo wrote: > >>>> I really think we need a way for (auto)pm and event polling to talk to > >>>> each other so that autopm can tell event poll to sod off while pm is > >>>> in effect. Trying to solve this from inside libata doesn't seem > >>>> right. The problem, again, seems to be figuring out which hardware > >>>> device maps to which block device. Hmmm... Any good ideas? > >>> > >>> A possible way of doing this is using pm qos. > >>> > >>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we > >>> can add another one: NO_POLL, use it like the following: > >>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no > >>> longer necessary. In the ZPODD's case, it should be set when the > >>> device is to be powered off; > >>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power > >>> is re-gained, this flag will be cleared. > >> > >> > >>> 3 In the disk_events_workfn, check if this flag is set, if so, simply > >>> return. > >> > >> It should be, skip calling disk->fops->check_events, but still queue the > >> work for next time's poll. > >> > >> -Aaron > >> > >>> > >>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer > >>> can access it through ata_device->sdev->sdev_gendev. > >>> > >>> Is this OK? > > > > No, I don't think so. PM QoS is about telling the layer that will put the > > device into low-power states what states are to be taken into consideration. > > In this case, however, we need to tell someone else that the device has been > > turned off. Clearly, we need a way to do that, but not through PM QoS. > > > > Did you consider using pm_runtime_suspended() to check the device status? > > The problem is, a device can be in runtime suspended state while still > needs to be polled... Well, maybe this is the problem, then? Why does it need to be polled when suspended? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote: > On 11/20/2012 02:00 PM, Aaron Lu wrote: > > On 11/19/2012 10:56 PM, Tejun Heo wrote: > >> I really think we need a way for (auto)pm and event polling to talk to > >> each other so that autopm can tell event poll to sod off while pm is > >> in effect. Trying to solve this from inside libata doesn't seem > >> right. The problem, again, seems to be figuring out which hardware > >> device maps to which block device. Hmmm... Any good ideas? > > > > A possible way of doing this is using pm qos. > > > > We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we > > can add another one: NO_POLL, use it like the following: > > 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no > > longer necessary. In the ZPODD's case, it should be set when the > > device is to be powered off; > > 2 Clear it when poll is necessary again. In the ZPODD's case, when power > > is re-gained, this flag will be cleared. > > > > 3 In the disk_events_workfn, check if this flag is set, if so, simply > > return. > > It should be, skip calling disk->fops->check_events, but still queue the > work for next time's poll. > > -Aaron > > > > > The disk->driverfs_dev can be used to host the pm qos flag, ATA layer > > can access it through ata_device->sdev->sdev_gendev. > > > > Is this OK? No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS. Did you consider using pm_runtime_suspended() to check the device status? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
On Monday, November 19, 2012 03:06:51 PM James Bottomley wrote: > On Mon, 2012-11-19 at 06:56 -0800, Tejun Heo wrote: > > Hey, Aaron. > > > > On Mon, Nov 19, 2012 at 11:09:40AM +0800, Aaron Lu wrote: > > > > What I'm confused about is what autopm does for devices w/o zpodd. > > > > What happens then? Is it gonna leave power on for the device and, > > > > say, go on to suspend the controller? But, how would that work for, > > > > say, future devices with async notification for media events? > > > > > > Maybe we shouldn't allow autopm for such devices? > > > > Yeah, maybe. It would be nice to be able to automatically power off > > disks and ports which aren't being used tho. > > > > > > That said, I can't say the snooping is pretty. It's a rather nasty > > > > thing to do. So, libata now wants information from the event polling > > > > in block layer, but reaching for block_device from ata_devices is > > > > nasty too. Hmmm... but aren't you already doing that to block polling > > > > on a powered down device? > > > > > > I was feeling brain damaged by this for some time :-) > > > > > > Basically, only ATA layer is aware of the power off thing, and sr knows > > > nothing about this(or it is not supposed to know this, at least, this is > > > what SCSI people think) and once powered off, I do not want the poll to > > > disturb the device, so I need to block the poll. I can't come up with > > > another way to achieve this except this nasty way. > > > > > > James suggests me to keep the poll, but emulate the command. The problem > > > with this is, the autopm for resume will kick in on each poll, so I'll > > > need to decide if power up the ODD for this time's resume is needed in > > > port's runtime resume callback. This made things complex and it also put > > > too much logic in the resume callback, which is not desired. And even if > > > I keep the ODD in powered off state by emulating this poll command, its > > > ancestor devices will still be resumed, and I may need to do some trick > > > in their resume callback to avoid needless power/state transitions. This > > > doesn't feel like an elegant way to solve this either. > > > > > > So yes, I'm still using this _nasty_ way to make the ODD stay in powered > > > off state as long as possible. But if there is other elegant ways to > > > solve this, I would appreciate it and happily using it. Personally, I > > > hope we can make sr aware of ZPODD, that would make the pain gone. > > > > I really think we need a way for (auto)pm and event polling to talk to > > each other so that autopm can tell event poll to sod off while pm is > > in effect. Trying to solve this from inside libata doesn't seem > > right. The problem, again, seems to be figuring out which hardware > > device maps to which block device. Hmmm... Any good ideas? > > I've asked the PM people several times about this, because it's a real > problem for almost everything: PM needs some type of top to bottom > stack view, which the layering isolation we have within storage really > doesn't cope with well. No real suggestion has been forthcoming. Actually, I think that the particular case in question is really special and the fact that there's the pollig loop that user space is involved in doesn't make things more stratightforward. And PM really doesn't need to see things top to bottom, but the polling needs to know what happens in the PM land. We need to be able to tell it "from now on tell user space that there are no events here". The question is where to put that information so that it's accessible to all parts of the stack involved. > The reason I think it should be emulated (in the acpi layer, not libata, > but as long as it's not in SCSI, I'm not so fussed where it ends up) is > because ZPODD is the software equivalent of ZPREADY, which will be done > in hardware and will be effectively invisible to autopm in the same way > that SCSI (and ATA) power management is mostly invisible. If we > currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact > ZPREADY emulation), we won't care (except for flipping the sofware bit) > whether the device support ZPODD or ZPREADY and it will all just > work(tm). The industry expectation is that ZPODD is just a transition > state between current power management and ZPREADY. Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles transparently, but still it won't save as much energy as it can. We'll need to do something about the polling in that case too, it seems. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v4 0/5] Migrate SCSI drivers to use dev_pm_ops
Hi James, On Friday, November 09, 2012 03:27:50 PM Aaron Lu wrote: > This patchset has been quiet for a while, so resend them. > > v4: > Only Patch 4 is modified: > Fixed a line over 80 characters warning by checkpatch.pl; > Update the changelog so that it is no more a try :-) > > v3: > Only patch 4 is modified: > Remove the special case for system freeze in scsi_bus_suspend_common > as pointed out by Alan Stern; > Updated some comments; > Removed the use of typedef (*pm_callback_t)(struct device *). > > v2: > Change the runtime suspend behaviour of sd driver by putting the device > into stopped power state. > Revert 2 patches which are no longer needed as pointed out by Alan Stern. > Find out device callbacks in bus callbacks as suggested by Alan Stern. > > Due to these changes, patch number grows from 2 -> 5. > > v1: > The 2 patches will migrate SCSI drivers to use the pm callbacks defined > in dev_pm_ops as pm_message is deprecated and should not be used by driver. > Bus level callback is changed to use callbacks defined in dev_pm_ops when > needed and sd's pm callback is updated to use what are defined in dev_pm_ops. > > > Aaron Lu (5): > sd: put to stopped power state when runtime suspend > Revert "[SCSI] scsi_pm: set device runtime state before parent > suspended" > Revert "[SCSI] runtime resume parent for child's system-resume" > pm: use callbacks from dev_pm_ops for scsi devices > sd: update sd to use the new pm callbacks > > drivers/scsi/scsi_pm.c | 98 > +++--- > drivers/scsi/sd.c | 18 +++--- > 2 files changed, 67 insertions(+), 49 deletions(-) Do you have any plans with respect to this patchset? It has been acked by Alan and me, do you have any objections against it? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/5] Migrate SCSI drivers to use dev_pm_ops
On Friday 12 of October 2012 11:07:18 Alan Stern wrote: > On Fri, 12 Oct 2012, Aaron Lu wrote: > > > v3: > > Only patch 4 is modified: > > Remove the special case for system freeze in scsi_bus_suspend_common > > as pointed out by Alan Stern; > > Updated some comments; > > Removed the use of typedef (*pm_callback_t)(struct device *). > > > > v2: > > Change the runtime suspend behaviour of sd driver by putting the device > > into stopped power state. > > Revert 2 patches which are no longer needed as pointed out by Alan Stern. > > Find out device callbacks in bus callbacks as suggested by Alan Stern. > > > > Due to these changes, patch number grows from 2 -> 5. > > > > v1: > > The 2 patches will migrate SCSI drivers to use the pm callbacks defined > > in dev_pm_ops as pm_message is deprecated and should not be used by driver. > > Bus level callback is changed to use callbacks defined in dev_pm_ops when > > needed and sd's pm callback is updated to use what are defined in > > dev_pm_ops. > > > > Aaron Lu (5): > > sd: put to stopped power state when runtime suspend > > Revert "[SCSI] scsi_pm: set device runtime state before parent > > suspended" > > Revert "[SCSI] runtime resume parent for child's system-resume" > > pm: use callbacks from dev_pm_ops for scsi devices > > sd: update sd to use the new pm callbacks > > > > drivers/scsi/scsi_pm.c | 97 > > +++--- > > drivers/scsi/sd.c | 18 +++--- > > 2 files changed, 66 insertions(+), 49 deletions(-) > > Remember to run all the patches through checkpatch.pl before submitting > them. Aside from that, for the whole series: > > Acked-by: Alan Stern >From me too: Acked-by: Rafael J. Wysocki And please do the checkpatch.pl thing as Alan said. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] pm: use callbacks from dev_pm_ops for scsi devices
pm_runtime_disable(dev);
> @@ -102,26 +96,49 @@ static int scsi_bus_prepare(struct device *dev)
>
> static int scsi_bus_suspend(struct device *dev)
> {
> - return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + return scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
> +}
> +
> +static int scsi_bus_resume(struct device *dev)
> +{
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + return scsi_bus_resume_common(dev, pm ? pm->resume : NULL);
> }
>
> static int scsi_bus_freeze(struct device *dev)
> {
> - return scsi_bus_suspend_common(dev, PMSG_FREEZE);
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + return scsi_bus_suspend_common(dev, pm ? pm->freeze : NULL);
> +}
> +
> +static int scsi_bus_thaw(struct device *dev)
> +{
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + return scsi_bus_resume_common(dev, pm ? pm->thaw : NULL);
> }
>
> static int scsi_bus_poweroff(struct device *dev)
> {
> - return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + return scsi_bus_suspend_common(dev, pm ? pm->poweroff : NULL);
> +}
> +
> +static int scsi_bus_restore(struct device *dev)
> +{
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + return scsi_bus_resume_common(dev, pm ? pm->restore : NULL);
> }
>
> #else /* CONFIG_PM_SLEEP */
>
> -#define scsi_bus_resume_common NULL
> #define scsi_bus_prepare NULL
> #define scsi_bus_suspend NULL
> +#define scsi_bus_resume NULL
> #define scsi_bus_freeze NULL
> +#define scsi_bus_thawNULL
> #define scsi_bus_poweroffNULL
> +#define scsi_bus_restore NULL
>
> #endif /* CONFIG_PM_SLEEP */
>
> @@ -130,10 +147,11 @@ static int scsi_bus_poweroff(struct device *dev)
> static int scsi_runtime_suspend(struct device *dev)
> {
> int err = 0;
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> dev_dbg(dev, "scsi_runtime_suspend\n");
> if (scsi_is_sdev_device(dev)) {
> - err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
> + err = scsi_dev_type_suspend(dev, pm ? pm->runtime_suspend :
> NULL);
> if (err == -EAGAIN)
> pm_schedule_suspend(dev, jiffies_to_msecs(
> round_jiffies_up_relative(HZ/10)));
> @@ -147,10 +165,11 @@ static int scsi_runtime_suspend(struct device *dev)
> static int scsi_runtime_resume(struct device *dev)
> {
> int err = 0;
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> dev_dbg(dev, "scsi_runtime_resume\n");
> if (scsi_is_sdev_device(dev))
> - err = scsi_dev_type_resume(dev);
> + err = scsi_dev_type_resume(dev, pm ? pm->runtime_resume : NULL);
>
> /* Insert hooks here for targets, hosts, and transport classes */
>
> @@ -229,11 +248,11 @@ void scsi_autopm_put_host(struct Scsi_Host *shost)
> const struct dev_pm_ops scsi_bus_pm_ops = {
> .prepare = scsi_bus_prepare,
> .suspend = scsi_bus_suspend,
> - .resume = scsi_bus_resume_common,
> + .resume = scsi_bus_resume,
> .freeze = scsi_bus_freeze,
> - .thaw = scsi_bus_resume_common,
> + .thaw = scsi_bus_thaw,
> .poweroff = scsi_bus_poweroff,
> - .restore = scsi_bus_resume_common,
> + .restore = scsi_bus_restore,
> .runtime_suspend = scsi_runtime_suspend,
> .runtime_resume = scsi_runtime_resume,
> .runtime_idle = scsi_runtime_idle,
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Tuesday 09 of October 2012 15:20:39 Aaron Lu wrote: > On 10/08/2012 06:21 PM, James Bottomley wrote: > > On Mon, 2012-10-08 at 17:27 +0800, Aaron Lu wrote: > >> On Sun, Sep 30, 2012 at 03:43:27PM -0400, Alan Stern wrote: > >>> On Sun, 30 Sep 2012, Jeff Garzik wrote: > >>> > >>>> The simple fact of "only ZPODD devices out there are ATA" is not the > >>>> decision-maker for where the code should live. It is more a question > >>>> where ZPODD belongs in the device/command set model currently employed. > >>> > >>> I don't really accept this argument. It's a little like saying: The > >>> tty layer uses ioctl commands to control RS232 line settings; therefore > >>> RS232 settings should be handled in the VFS layer as part of the ioctl > >>> core. > >>> > >>> Regardless, according to Aaron the ZP power-off stuff is currently > >>> implemented only in ACPI, tied more closely to the ATA layer than the > >>> SCSI layer (though not part of either). It is not part of the SCSI > >>> spec in any form. > >> > >> The mechanism of SATA ODD zero power model is specified in Mount Fuji > >> spec v8 section 15 describing what the ODD needs support, how to sense > >> if the ODD is ready to be powered off and on power up what needs to be > >> done, etc. And the actual power off and wakeup is implemented in ACPI > >> and SATA. > >> > >>> Now it's true that determining whether a device is > >>> in the right state for power to be removed involves sending a TEST UNIT > >>> READY command, which is of course a SCSI command. This seems to be > >>> incidental to the overall scheme, however. > >> > >> I need to add that, there are 2 schemes to sense if the ODD is ready to > >> be powered off: > >> 1 the one I used here, by checking if the door is closed and no media > >> inside with test_unit_ready; > >> 2 some ZP capable ODDs can report zero power ready(ZPReady) event to > >> host when queried, eliminating the need of host checking the conditions. > > > > The way I read the standard is that ZP ODD is a hack to try and get to > > Thanks for your time. > > > off and ZPready is the same hack but integrated into the standardised > > power management states (and hence available to normal power saving). > > The standard even implies ZP ODD is a less desirable hack by > > recommending the use of ZPready. > > There are ZPODDs not supporting ZPready and I want to support them so > the sense scheme 1 is used. > > > > > The ZPready method, being an extension of the usual SCSI power > > management model, is pretty much what we support today (especially as > > the whole thing is timer driven from values in the mode page and happens > > pretty much invisibly to us). > > > > Since the object is to make this as painless as possible, why don't we > > just implement ZPODD the way the spec recommends? i.e. emulate the > > timers at an incredibly low level and intercept and emulate the non-disk > > commands listed in table 321. I bet, in Linux, since we moved basically > > to GET_EVENT_STATUS_NOTIFICATION, that's the only one that actually > > needs an emulation. > > > > The state model seems to work if you snoop the polled media event, so > > you wait for no media, then set your internal timer, stop it if we get a > > media change and power off the device after interval expiry. Thereafter > > you emulate media event with no change keeping the device powered off. > > If a disc gets inserted or the eject button is pressed, you see that via > > the SATA PHY events (so wake the drive) and if the Upper Layer sends an > > unexpected command, you also power on the drive. > > > > That way all of this should be nicely containable within SATA/ACPI. > > Thanks for the suggestion, it is really something that I've never > thought of :-) Well, that's what I wanted to say previously, but James expressed it much better than I could. ;-) > But I was hoping to use the runtime pm framework to support ZPODD. > With your suggestion, I don't know how to do this. Maybe I can set the > scsi device representing the ODD to runtime suspended once I decided to > power it off and resume it when I power it up. But there is a problem, > that I'm setting a scsi device's runtime status in ATA layer, this > doesn't feel right. And someday, someone may want to add runtime pm > support for sr and the runtime
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Saturday, September 29, 2012, Aaron Lu wrote: > On 09/29/2012 10:29 PM, Alan Stern wrote: > > On Sat, 29 Sep 2012, Aaron Lu wrote: > > > >>> I don't think this is a good idea, quite frankly. sr seems to be a too > >>> generic place for that. > >> > >> Does this mean sr can only have code that is useful to all devices it > >> manages? i.e. If a piece of code enables a feature for a special kind of > >> ODD(like the sata based ZPODD), it shouldn't be done in sr? > > > > Drivers are allowed to have special features and quirks that apply only > > to some devices. > > I think SATA based zero power capable ODDs are the "some devices". > > > > >> There is nothing in theory stopping us from doing this in ata layer. > >> For the loading mechanism, we can always send an ATAPI command to figure > >> it out. > >> > >> So gentlemen, I need your opinions on where this ZPODD code should live > >> before I can continue this work, thanks. > > > > Can arbitrary SCSI devices be ZP, or does this notion apply only to > > ATAPI-based drives? That's the key question, and the answer determines > > where the ZP support belongs. > > I don't know if arbitrary SCSI devices can be ZP or not, the SPC spec > doesn't seem to define such a power state. > > ZPODD is defined for sata based ATAPI ODD which supports device > attention, but doesn't specify how the ODD is powered off and how the > device attention pin notifies OS. On x86 systems, these are implemented > by ACPI. > > Though SCSI devices may not have a general notion of ZP, the fact that > ZPODD are managed by sr driver is enough to make the decision that ZPODD > code can live in sr? Well, only a part of it is handled by sr, the high-level part so to speak. The low-level handling is done by the ATA layer. Now, since sr is the high-level part and is supposed to be generic, I don't think it's appropriate to make it care about some low-level things specific to ATA (because there is another layer of code supposed to handle those). > > On the other hand, the sr driver certainly deserves to have some form > > of runtime PM support, even for devices that aren't ZP. > > Agree. > > And the following codes need to find a home: > - Code used to handle ACPI wake notification(currently done in ATA, but > causes the "annoying" need_eject flag in scsi_device); And quite frankly I'd more and more convinced that this flag isn't really necessary. What you really want to achieve is to eject the tray of a tray-type ODD if the eject is signaled through a GPE. I don't see anything for sr to do in that. Moreover, you probably would like to do that even if the drive is not powered down, right? I wonder if this mechanism can be used for user space notification without polling regardless of whether or not the zero-power feature is used? > - Code to check if the ODD is ready to be powered off per the ZPODD > spec. If that can be done at the ATA level, I'd prefer it too. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Saturday, September 29, 2012, Alan Stern wrote: > On Sat, 29 Sep 2012, Aaron Lu wrote: > > > > I don't think this is a good idea, quite frankly. sr seems to be a too > > > generic place for that. > > > > Does this mean sr can only have code that is useful to all devices it > > manages? i.e. If a piece of code enables a feature for a special kind of > > ODD(like the sata based ZPODD), it shouldn't be done in sr? > > Drivers are allowed to have special features and quirks that apply only > to some devices. > > > There is nothing in theory stopping us from doing this in ata layer. > > For the loading mechanism, we can always send an ATAPI command to figure > > it out. > > > > So gentlemen, I need your opinions on where this ZPODD code should live > > before I can continue this work, thanks. > > Can arbitrary SCSI devices be ZP, or does this notion apply only to > ATAPI-based drives? That's the key question, and the answer determines > where the ZP support belongs. I agree. That said for now I'm not aware of any other ZP devices. It also is unclear whether or not their requirements would be the same for the ZPODD devices. > On the other hand, the sr driver certainly deserves to have some form > of runtime PM support, even for devices that aren't ZP. Yes, it does, but it is unclear to me at this point what it should do in its callbacks. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Saturday, September 29, 2012, Aaron Lu wrote: > [Adding more people and list back in] > > On 09/29/2012 05:46 AM, Rafael J. Wysocki wrote: > > On Friday, September 28, 2012, Aaron Lu wrote: > >> On 09/28/2012 07:15 AM, Rafael J. Wysocki wrote: > >>> On Thursday, September 27, 2012, Aaron Lu wrote: > >>>> On 09/27/2012 05:37 AM, Rafael J. Wysocki wrote: > >>>>> > >>>>> Say the user has pressed the eject button. What does need to happen so > >>>>> that > >>>>> the tray is physically ejected? > >>>> > >>>> The tray is ejected by the ODD itself, host does not have to do anything. > >>>> > >>>> There is a command(PREVENT_MEDIUM_REMOVAL) to lock the door so that when > >>>> user presses the eject button, the tray will not be ejected. This command > >>>> is usually sent when we have a disc inside and a user space program > >>>> opened the underlying block device(e.g. /dev/sr0) to read/write data. > >>>> > >>>> And host can also eject the tray by sending a START_STOP_UNIT command > >>>> with param LoEj set to 1 and we have a function called sr_tray_move to > >>>> do just this. And this is also what I've used to eject the tray after > >>>> user wakes up the ODD, as when user presses the eject button when the > >>>> ODD is in zero power state, it can't eject the tray as usual, so host > >>>> software will need to do this, that's the reason I need to know such > >>>> information: > >>>> When ODD is resumed, is it because user wakes it up? > >>> > >>> But START_STOP_UNIT eventually causes ata_scsi_start_stop_xlat() to be > >> > >> You are following ata case, while the ODD is an atapi device :-) > >> The translation function is atapi_xlat, but that doesn't affect the idea > >> here. > >> > >>> executed, so I wonder if we really need to go up through the SCSI stack > >>> to send that command to the drive from there? It should be possible > >>> to issue STANDBY/READ VERIFY to the device directly from libata if > >>> an eject event is signaled through a GPE. > >> > >> Yes, this is possible. > >> Though it doesn't feel very cool, since I have no idea if the ODD is a > >> tray type or slot type in ATA layer and I'll blindly send this command > >> to it then, not a problem maybe. > > > > It would be good to verify if that works for slot devices, if possible. > > The ACPI GPE event is triggered when user inserts a disc into a slot > type ODD, and if I send an eject command to it, the disc will be > ejected, which is wrong. > > I need to know the loading mechanism(tray type or slot type) of the ODD > to decide if I should send this command. > > > > >> And what do you think of moving the acpi notification code to sr? > >> http://marc.info/?l=linux-pm&m=134873904332704&w=4 > > > > I don't think this is a good idea, quite frankly. sr seems to be a too > > generic place for that. > > Does this mean sr can only have code that is useful to all devices it > manages? i.e. If a piece of code enables a feature for a special kind of > ODD(like the sata based ZPODD), it shouldn't be done in sr? If the feature is specific to one special kind of ODD only, then I don't think sr is the right place to add support for it. > > Ideally, the whole ZPODD handling should not be visible to the SCSI layer, > > I can see 2 problems: > - Don't know its loading machanism so we have the problem above; Does using the need_eject flag address this problem somehow? > - Need to send command to find out if ODD is zero power ready somewhere > in ata layer, this implies the device is doing IO after it is runtime > suspended in scsi layer. There's nothing wrong with accessig suspended devices as long as we know that they will respond. :-) > > perhaps except the "no_polling" flag disabling the polling that may be > > useful for other purposes in principle. > > I hope so, let's hear what other people has to say. > > > > > I'm not sure if it's possible at this point, but if not we need to know > > very precisely why not. > > There is nothing in theory stopping us from doing this in ata layer. > For the loading mechanism, we can always send an ATAPI command to figure > it out. > > So gentlemen, I need your opinions on where this ZPODD code should live > before I can continue this work, thanks. I would _try_ to add it at the ATA level. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)
On Thursday, September 27, 2012, Aaron Lu wrote:
> On 09/27/2012 10:42 PM, Alan Stern wrote:
> > On Thu, 27 Sep 2012, Aaron Lu wrote:
> >
> >>> Moreover, I'd like to migrate SCSI drivers to the PM handling based on
> >>> struct
> >>> dev_pm_ops eventually and your change is kind of going in the opposite
> >>> direction. I don't know how much effort the migration is going to take,
> >>> though, so perhaps we can just make this change first.
> >>
> >> Does the following change look OK?
> >>
> >> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> >> index dc0ad85..1fb7ccc 100644
> >> --- a/drivers/scsi/scsi_pm.c
> >> +++ b/drivers/scsi/scsi_pm.c
> >> @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev)
> >>
> >>dev_dbg(dev, "scsi_runtime_suspend\n");
> >>if (scsi_is_sdev_device(dev)) {
> >> - err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
> >> + err = scsi_device_quiesce(to_scsi_device(dev));
> >> + if (err)
> >> + goto out;
> >> +
> >> + err = pm_generic_runtime_suspend(dev);
> >> + if (!err)
> >> + goto out;
> >> +
> >> + scsi_device_resume(to_scsi_device(dev));
> >>if (err == -EAGAIN)
> >>pm_schedule_suspend(dev, jiffies_to_msecs(
> >>round_jiffies_up_relative(HZ/10)));
> >
> > Maybe in the end it will be better, but for now this looks like
> > unnecessary code duplication. Basically you are copying
> > scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you
> > omitted the debugging statement.
>
> And I've used pm_generic_runtime_suspend :-)
> That would allow me to write runtime callbacks of dev_pm_ops for
> indivisual scsi drivers, like sr.
Well, actually, I meant something different.
The above patch would make it possible for drivers to provide runtime PM
callbacks through dev->driver.pm, but drv->suspend and drv->resume would
still be used for system suspend/resume (and hibernation).
The transition to struct dev_pm_ops I meant, however, would be to start using
callbacks from dev->driver.pm for _all_ device PM operations by default and
fall back to drv->suspend and drv->resume for the drivers whose dev->driver.pm
is NULL (along the lines of what PCI does). The next step would be to
convert all drivers to use dev->driver.pm only.
So I wouldn't like any drivers to use dev->driver.pm callbacks for some
operations and drv->suspend and drv->resume for the remaining ones at the
same time.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Wednesday, September 26, 2012, Aaron Lu wrote: > On Tue, Sep 25, 2012 at 11:45:34PM +0200, Rafael J. Wysocki wrote: > > On Tuesday, September 25, 2012, Aaron Lu wrote: > > > On 09/25/2012 10:23 PM, Oliver Neukum wrote: > > > > On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote: > > > >> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote: > > > >>> On Tuesday, September 25, 2012, Aaron Lu wrote: > > > >>>> I'm thinking of enabling this GPE in sr_suspend once we decided that > > > >>>> it > > > >>>> is ready to be powered off, so the time frame between sr_suspend and > > > >>>> when the power is actually removed in libata should be taken care of > > > >>>> by > > > >>>> the GPE. If GPE fires, the notification function will request a > > > >>>> runtime > > > >>>> resume of the device. Does this sound OK? > > > >>> > > > >>> Well, depending on the implementation. sr_suspend() should be rather > > > >>> generic, but the ACPI association (including the GPE thing) is > > > >>> specific to ATA. > > > >> > > > >> Sorry, but don't quite understand this. > > > >> > > > >> We have ACPI bindings for scsi devices, isn't that for us to use ACPI > > > >> when needed in scsi? > > > > > > > > We don't have ACPI bindings for generic SCSI devices. We have such > > > > bindings for SATA drives. You can put such things only in sr if it > > > > applies > > > > to all (maybe most) types of drives. > > > > > > OK. Then these scsi bindings for sata drives will be pretty much of > > > no use I think. > > > > > > > > > > >> BTW, if sr_suspend should be generic, that would suggest I shouldn't > > > >> write any ZPODD related code there, right? Any suggestion where these > > > >> code should go then? > > > > > > > > libata. Maybe some generic hooks can be called in sr_suspend(). > > > > > > Thanks for the suggestion. > > > The problem is, I need to know whether the door is closed and if there > > > is a medium inside. I've no way of getting such information in libata. > > > > How does sr get to know it in the libata case? > > By executing a test_unit_ready command. > > Libata does/should not have any routine to do this, it is one of the > transport of SCSI devices and it relies on SCSI driver to manage the > device(both disk and ODD). > > > > > > > PS: Are you sure sr_suspend() handles DVD-RAMs correctly? > > > > > > No. Is there a spec for it? > > > Considering there are many different drives sr handle, is it possible to > > > write a generic sr_suspend? > > > Maybe your suggestion of callback is the way to go. > > > What about this idea, if we find this is a ZPODD capable drive, we > > > enable runtime suspend for it and write a suspend callback according to > > > ZPODD spec. For other drives that does not have a suspend callback, we > > > do not enable runtime suspend. > > > > You can enable runtime PM for all kinds of drives, but make the suspend > > and resume callbacks only do something for ZPODD ones. This may allow their > > parents to use runtime PM (as Alan said earlier in this thread), even if the > > drives themseleves are not really physically suspended. > > Sounds good. > > > > > > Does this sound reasonable? > > > > First, we need to know when the drive is not in use. That information > > we can get from the sr's runtime PM and it looks like we need to notify > > libata about that somehow. I'm not sure what mechanism is the best for > > that at the moment. > > The current mechanism to notify libata is by rumtime suspend. When scsi > device is runtime suspended, its parent device will be suspended. And > ata_port is one of the ancestor devices of scsi device, and we will > remove its power in ata_port's runtime suspend callback. The problem, then, is that the ata_port's runtime suspend callback would have to know whether or not power can be removed from the drive. I'm going to post patches introducing a "no power off" flag for PM QoS, among other things, today or tomorrow. I suppose this flag might be used to tell the ata_port's suspend not to remove power from the attached device. > > Second, when the device is resumed by remote wakeup, we need to notify > > sr about that. A "resume" alone is not sufficient, though, because it may > > be necessary to open the tray. Perhaps in that case we can use the same > > mechanism by which user events are processed by libata and delivered to sr? > > Thanks for the suggestion. > I'm not aware of any user events processed by libata. Do you mean the > events_checking poll? Yes, basically. In the remote wakeup case libata might report the same status as in the "user pressed the eject button" situation happening normally with power on. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Tuesday, September 25, 2012, Aaron Lu wrote: > On 09/25/2012 10:23 PM, Oliver Neukum wrote: > > On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote: > >> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote: > >>> On Tuesday, September 25, 2012, Aaron Lu wrote: > >>>> I'm thinking of enabling this GPE in sr_suspend once we decided that it > >>>> is ready to be powered off, so the time frame between sr_suspend and > >>>> when the power is actually removed in libata should be taken care of by > >>>> the GPE. If GPE fires, the notification function will request a runtime > >>>> resume of the device. Does this sound OK? > >>> > >>> Well, depending on the implementation. sr_suspend() should be rather > >>> generic, but the ACPI association (including the GPE thing) is specific > >>> to ATA. > >> > >> Sorry, but don't quite understand this. > >> > >> We have ACPI bindings for scsi devices, isn't that for us to use ACPI > >> when needed in scsi? > > > > We don't have ACPI bindings for generic SCSI devices. We have such > > bindings for SATA drives. You can put such things only in sr if it applies > > to all (maybe most) types of drives. > > OK. Then these scsi bindings for sata drives will be pretty much of > no use I think. > > > > >> BTW, if sr_suspend should be generic, that would suggest I shouldn't > >> write any ZPODD related code there, right? Any suggestion where these > >> code should go then? > > > > libata. Maybe some generic hooks can be called in sr_suspend(). > > Thanks for the suggestion. > The problem is, I need to know whether the door is closed and if there > is a medium inside. I've no way of getting such information in libata. How does sr get to know it in the libata case? > > PS: Are you sure sr_suspend() handles DVD-RAMs correctly? > > No. Is there a spec for it? > Considering there are many different drives sr handle, is it possible to > write a generic sr_suspend? > Maybe your suggestion of callback is the way to go. > What about this idea, if we find this is a ZPODD capable drive, we > enable runtime suspend for it and write a suspend callback according to > ZPODD spec. For other drives that does not have a suspend callback, we > do not enable runtime suspend. You can enable runtime PM for all kinds of drives, but make the suspend and resume callbacks only do something for ZPODD ones. This may allow their parents to use runtime PM (as Alan said earlier in this thread), even if the drives themseleves are not really physically suspended. > Does this sound reasonable? First, we need to know when the drive is not in use. That information we can get from the sr's runtime PM and it looks like we need to notify libata about that somehow. I'm not sure what mechanism is the best for that at the moment. Second, when the device is resumed by remote wakeup, we need to notify sr about that. A "resume" alone is not sufficient, though, because it may be necessary to open the tray. Perhaps in that case we can use the same mechanism by which user events are processed by libata and delivered to sr? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Tuesday, September 25, 2012, Aaron Lu wrote: > On Mon, Sep 24, 2012 at 11:40:18PM +0200, Rafael J. Wysocki wrote: > > On Monday, September 24, 2012, Aaron Lu wrote: > > > On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote: > > > > And I'd add a comment about the next poll. > > > > > > > > This appears somewhat racy, though, because in theory a media may be > > > > inserted > > > > while we are removing power from the device. Isn't that a problem? > > > > > > Yes, this is a problem. > > > To avoid this race, I think the following needs to be done: > > > - For slot type ODD, make it such that user can't insert a disc; > > > - For tray type ODD, make it such that when user presses the eject > > > button, the tray doesn't open. > > > I'll see if this is possible, thanks for the remind. > > > > OK > > Looks like this is not the right thing to do, if I lock the door user > will be confused. > > > > > > > > The poll runs periodically, until the device is powered off(reflected > > > > > by > > > > > the powered_off flag), in which case, the poll will simply return > > > > > 0 without touching this device. > > > > > > > > Is it useful to poll the device after it has been suspended, but not > > > > powered > > > > off? > > > > > > Yes, it is necessary to poll the device when it has been suspended with > > > power left. The reason is, we need to check if there is a media change > > > event happened, and the way to check this is to issue a > > > GET_EVENT_STATUS_NOTIFICATION comand. > > > > > > Please note that when the drive is not powered off, it will not send us > > > a notification when its eject button is pressed. > > > > I'm not sure about that, actually. If it doesn't notify us, that whole > > thing > > is inherently racy pretty much beyond fixing, because it is always possible > > that the user will press the eject button right after we've checked the > > status last time and right before power removal. We're going to lose that > > event, so the user will have to push the button once again in that case. > > I just checked the spec again and tested, when the ODD has power, it > will also send out notifications on pressing the eject button/inserting > a disc. So we should be able to capture such a event. Good! > > > > > That's correct. > > > > > AFAIK, user space does not care how often the device is polled, it > > > > > cares only one thing: when there is a media change event, please let > > > > > me > > > > > know(through uevent). > > > > > > > > So that's why we do the polling, right? > > > > > > Yes. > > > > Well, that's difficult. > > > > If the remote wakeup is signaled through a GPE, it should be possible to > > enable it before we actually put the device into D3cold. That may allow us > > to eliminate the races. > > Thanks for the suggestion, I'll update the code. > > I'm thinking of enabling this GPE in sr_suspend once we decided that it > is ready to be powered off, so the time frame between sr_suspend and > when the power is actually removed in libata should be taken care of by > the GPE. If GPE fires, the notification function will request a runtime > resume of the device. Does this sound OK? Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/6] ZPODD patches
On Monday, September 24, 2012, Aaron Lu wrote: > On Mon, Sep 24, 2012 at 03:06:11PM +0200, Rafael J. Wysocki wrote: > > On Monday, September 24, 2012, Aaron Lu wrote: > > > On Fri, Sep 21, 2012 at 11:18:27PM +0200, Rafael J. Wysocki wrote: > > > > On Friday, September 21, 2012, Aaron Lu wrote: > > > > > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote: > > > > > > On Wednesday, September 19, 2012, Aaron Lu wrote: > > > > > > > Thanks Rafael, and if there is any question/problem, > > > > > > > please kindly let me know. > > > > > > > > > > > > Well, unfortunately my initial review indicates that the patchset > > > > > > is not > > > > > > quite ready to go upstream yet. > > > > > > > > > > > > I'll send comments in replies to the individual patches, but > > > > > > overall I can > > > > > > say that at this stage of development, when I look at the patches, > > > > > > it should > > > > > > be clear to me not only what is being changed, but _why_ it is > > > > > > being changed > > > > > > in the first place and, secondly, why it is being changed in this > > > > > > particular > > > > > > way. It's far from that, though. > > > > > > > > > > I'm adding zero power support for optical disk drive(ZPODD), which is > > > > > made possible with the newly defined device attention(DA) pin > > > > > introduced > > > > > in SATA 3.1 spec. > > > > > > > > > > The idea here is to use runtime pm to achieve this, so I basically > > > > > did 2 > > > > > things: > > > > > 1 Add runtime pm support for ODD; > > > > > 2 Add power off support for ODD after it is runtime suspended. > > > > > > > > > > Patch 2 is runtime pm support for ODD, the reason it is done this way > > > > > is > > > > > discussed here: > > > > > http://www.spinics.net/lists/linux-scsi/msg61551.html > > > > > > > > Why isn't it explained in the patch changelog, then? People should be > > > > able > > > > to learn why things are done the way they are done from git logs. > > > > > > > > > The basic idea is, the ODD will be runtime suspended as long as there > > > > > is > > > > > nobody using it, that is, no programs opening the block device. > > > > > > > > > > The ODD will be polled periodically, so it will be runtime resumed > > > > > before checking if there is any events pending and suspended when > > > > > done. > > > > > > > > OK. So what happens if we power off the drive via runtime PM. Does it > > > > it really make sense to resumie it through polling in that case? > > > > > > No, this is the reason I introduced the powered_off flag. If set, the > > > poll will simply return without touching the device. > > > > > > I've tried to do a disk_block_events call on its suspend callback when > > > it is ready to be powered off, but there is a race that I don't know how > > > to solve: > > > pm_runtime_suspenddisk_events_workfn > > > scsi_dev_type_suspend sr_block_check_events > > > sr_suspendcdrom_check_events > > > disk_block_events cdrom_update_events > > > (this call waits for allsr_check_events > > > running events_checking function scsi_autopm_get_device > > > to return) > > > > > > Suppose sr_suspend runs first, and then sr_check_events comes in. > > > sr_suspend calls disk_block_events, which waits for sr_check_events, > > > while scsi_autopm_get_device wait for suspend callback to finish, > > > deadlock. > > > > I need some more time to think about this, stay tuned. > > Thanks. Alan has just given you a good suggestion, you can follow it I think. > > > > > The only exception is, if we found a disc is just inserted, we will > > > > > not > > > > > idle it immediately at the end of the poll, reason explained in > > > > > another > > > > > mail. > > &
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Monday, September 24, 2012, Aaron Lu wrote:
> On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 24, 2012, Aaron Lu wrote:
> > > On Fri, Sep 21, 2012 at 10:49:32PM +0200, Rafael J. Wysocki wrote:
> > > > On Friday, September 21, 2012, Aaron Lu wrote:
> > > > > On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, September 12, 2012, Aaron Lu wrote:
[...]
> > > > > > > /* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
> > > > > > > if (!cd->tur_changed) {
> > > > > > > @@ -287,6 +294,12 @@ do_tur:
> > > > > > > cd->tur_changed = false;
> > > > > > > cd->get_event_changed = false;
> > > > > > >
> > > > > > > +out:
> > > > > > > + if (cd->media_present && !last_present)
> > > > > > > + pm_runtime_put_noidle(&cd->device->sdev_gendev);
> > > > > > > + else
> > > > > > > + scsi_autopm_put_device(cd->device);
> > > > > > > +
> > > > > >
> > > > > > This thing is asking for a comment.
> > > > > >
> > > > > > It looks like you're kind of avoiding to call _idle() for the
> > > > > > device, but why?
> > > > > > What might go wrong if pm_runtime_put() is used instead of the
> > > > > > whole conditional,
> > > > > > among other things?
> > > > >
> > > > > The above code means, if we found that a disc is just
> > > > > inserted(reflected
> > > > > by cd->media_present is true and last_present is false), we do not
> > > > > want
> > > > > to put the device into suspend state immediately until next poll. In
> > > > > the
> > > > > interval, some programs may decide to use this device by opening it.
> > > > >
> > > > > Nothing will go wrong, but it can possibly avoid a runtime status
> > > > > change.
> > > >
> > > > OK, so suppose the condition is true and we do the _noidle() put. Who's
> > > > going to suspend the device in that case if no one actually uses the
> > > > device?
> > >
> > > Next time when the check_events poll runs, it will find that:
> > > 1 Either the disc is still there, then both last_present and
> > > media_present would be true, we will do the put to idle it;
> > > 2 Or the disc is ejected, we will do the put to idle it.
> >
> > In that case I would do:
> >
> > pm_runtime_put_noidle(&cd->device->sdev_gendev);
> > if (cd->media_present && !last_present)
> > pm_runtime_suspend(&cd->device->sdev_gendev);
>
> This doesn't cover the !cd->media_present(media not present) case.
> If there is no media present, we will also need to idle it.
Oh, I got the condition backwards. I meant:
pm_runtime_put_noidle(&cd->device->sdev_gendev);
if (!cd->media_present || last_present)
pm_runtime_suspend(&cd->device->sdev_gendev);
which should be equivalent to your original code (if I'm not mistaken again).
> > And I'd add a comment about the next poll.
> >
> > This appears somewhat racy, though, because in theory a media may be
> > inserted
> > while we are removing power from the device. Isn't that a problem?
>
> Yes, this is a problem.
> To avoid this race, I think the following needs to be done:
> - For slot type ODD, make it such that user can't insert a disc;
> - For tray type ODD, make it such that when user presses the eject
> button, the tray doesn't open.
> I'll see if this is possible, thanks for the remind.
OK
> > > The poll runs periodically, until the device is powered off(reflected by
> > > the powered_off flag), in which case, the poll will simply return
> > > 0 without touching this device.
> >
> > Is it useful to poll the device after it has been suspended, but not powered
> > off?
>
> Yes, it is necessary to poll the device when it has been suspended with
> power left. The reason is, we need to check if there is a media change
> event happened, and the way to check this is to issue a
> GET_EVENT_STATUS_NOTIFICATION comand.
>
> Please note that when the drive is not powered off, it will not send us
> a notification when its eject bu
Re: [PATCH v7 0/6] ZPODD patches
On Monday, September 24, 2012, Aaron Lu wrote: > On Fri, Sep 21, 2012 at 11:18:27PM +0200, Rafael J. Wysocki wrote: > > On Friday, September 21, 2012, Aaron Lu wrote: > > > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote: > > > > On Wednesday, September 19, 2012, Aaron Lu wrote: > > > > > Thanks Rafael, and if there is any question/problem, > > > > > please kindly let me know. > > > > > > > > Well, unfortunately my initial review indicates that the patchset is not > > > > quite ready to go upstream yet. > > > > > > > > I'll send comments in replies to the individual patches, but overall I > > > > can > > > > say that at this stage of development, when I look at the patches, it > > > > should > > > > be clear to me not only what is being changed, but _why_ it is being > > > > changed > > > > in the first place and, secondly, why it is being changed in this > > > > particular > > > > way. It's far from that, though. > > > > > > I'm adding zero power support for optical disk drive(ZPODD), which is > > > made possible with the newly defined device attention(DA) pin introduced > > > in SATA 3.1 spec. > > > > > > The idea here is to use runtime pm to achieve this, so I basically did 2 > > > things: > > > 1 Add runtime pm support for ODD; > > > 2 Add power off support for ODD after it is runtime suspended. > > > > > > Patch 2 is runtime pm support for ODD, the reason it is done this way is > > > discussed here: > > > http://www.spinics.net/lists/linux-scsi/msg61551.html > > > > Why isn't it explained in the patch changelog, then? People should be able > > to learn why things are done the way they are done from git logs. > > > > > The basic idea is, the ODD will be runtime suspended as long as there is > > > nobody using it, that is, no programs opening the block device. > > > > > > The ODD will be polled periodically, so it will be runtime resumed > > > before checking if there is any events pending and suspended when done. > > > > OK. So what happens if we power off the drive via runtime PM. Does it > > it really make sense to resumie it through polling in that case? > > No, this is the reason I introduced the powered_off flag. If set, the > poll will simply return without touching the device. > > I've tried to do a disk_block_events call on its suspend callback when > it is ready to be powered off, but there is a race that I don't know how > to solve: > pm_runtime_suspenddisk_events_workfn > scsi_dev_type_suspend sr_block_check_events > sr_suspendcdrom_check_events > disk_block_events cdrom_update_events > (this call waits for allsr_check_events > running events_checking function scsi_autopm_get_device > to return) > > Suppose sr_suspend runs first, and then sr_check_events comes in. > sr_suspend calls disk_block_events, which waits for sr_check_events, > while scsi_autopm_get_device wait for suspend callback to finish, > deadlock. I need some more time to think about this, stay tuned. > > > The only exception is, if we found a disc is just inserted, we will not > > > idle it immediately at the end of the poll, reason explained in another > > > mail. > > > > > > This is the rational I wrote patch 2, and patch 1 is used by patch 2. > > > > > > Patch 3 is adding power off support for ODD after it is runtime > > > suspended, the condition is specified in section 15: > > > ftp://ftp.seagate.com/sff/INF-8090.PDF > > > > > > That is, for tray type ODD: no media inside and door closed; for slot > > > type ODD: no media inside. > > > > > > The is the reason sr_suspend is written, for non-ZPODD capable devices, > > > it does nothing; for ZPODD devices, it will check the above condition to > > > see if it is ready to be powered off. The ready_to_power_off flag will be > > > used by ATA layer to decide if power can be removed. > > > > Now, James says he doesn't like the way ready_to_power_off is used. Sure > > enough, it is totally irrelevant to the majority of SCSI devices. It > > actually > > is totally irrelevant to everything in the SCSI subsystem except for the sr > > driver and libata
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Monday, September 24, 2012, Aaron Lu wrote: > On Fri, Sep 21, 2012 at 10:49:32PM +0200, Rafael J. Wysocki wrote: > > On Friday, September 21, 2012, Aaron Lu wrote: > > > On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote: > > > > On Wednesday, September 12, 2012, Aaron Lu wrote: > > > > > Place the ODD into runtime suspend state as soon as there is nobody > > > > > using it. > > > > > > > > OK, so how is ODD related to the sr driver? > > > > > > As Alan has explained, ODD(optical disk drive) is driven by scsi > > > sr driver. > > > > OK, but what about writing "ODD (Optical Disk Drive)" in the changelog? > > > > People reading git logs may not know all of the hardware acronyms and the > > "0" message doesn't go into the git log. :-) > > > > > > > The only exception is, if we just find that a new medium is > > > > > inserted, we wait for the next events checking to idle it. > > > > > > > > What exactly do you mean by "to idle it"? > > > > > > I mean to put its usage count so that its idle callback will kick in. > > > > So I'd just write that directly in the changelog. > > > > > > Does this patch have any functional effect without the following > > > > patches? > > > > > > Yes, this one alone takes care of ODD's runtime pm > > > > I suppose you mean the runtime PM status and usage counter? I.e. the > > "software > > state"? > > Yes. > > > > > > while the following > > > patches take care of removing its power after it's runtime suspended. > > > But it doesn't have any real benefit without the following patches. > > > > Please put that information into the changelog too. > > As Alan explained, I think I would say: > Though currently it doesn't have any benefit, it allows its parent > devices enter runtime suspend state which may save some power. Well, please say that in the changelog, then. :-) > > > > > Based on ideas of Alan Stern and Oliver Neukum. > > > > > > > > > > Signed-off-by: Aaron Lu > > > > > --- > > > > > drivers/scsi/sr.c | 29 + > > > > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c > > > > > index 5fc97d2..7a8222f 100644 > > > > > --- a/drivers/scsi/sr.c > > > > > +++ b/drivers/scsi/sr.c > > > > > @@ -45,6 +45,7 @@ > > > > > #include > > > > > #include > > > > > #include > > > > > +#include > > > > > #include > > > > > > > > > > #include > > > > > @@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct > > > > > gendisk *disk) > > > > > kref_get(&cd->kref); > > > > > if (scsi_device_get(cd->device)) > > > > > goto out_put; > > > > > + if (scsi_autopm_get_device(cd->device)) > > > > > + goto out_pm; > > > > > goto out; > > > > > > > > Why don't you do > > > > > > > > > + if (!scsi_autopm_get_device(cd->device)) > > > > > + goto out; > > > > > > > > without the new label? > > > > > > I was just stupidly following the pattern. > > > Thanks and I'll change this. > > > > > > > > > > > > > > > > > + out_pm: > > > > > + scsi_device_put(cd->device); > > > > > out_put: > > > > > kref_put(&cd->kref, sr_kref_release); > > > > > cd = NULL; > > > > > @@ -163,6 +168,7 @@ static void scsi_cd_put(struct scsi_cd *cd) > > > > > mutex_lock(&sr_ref_mutex); > > > > > kref_put(&cd->kref, sr_kref_release); > > > > > scsi_device_put(sdev); > > > > > + scsi_autopm_put_device(sdev); > > > > > mutex_unlock(&sr_ref_mutex); > > > > > } > > > > > > > > > > @@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct > > > > > cdrom_device_info *cdi, > > > > >
Re: [PATCH v7 0/6] ZPODD patches
On Saturday, September 22, 2012, Alan Stern wrote:
> On Sat, 22 Sep 2012, Rafael J. Wysocki wrote:
>
> > > > I see. So the sr's runtime suspend may be useful even without the
> > > > power-off
> > > > feature, right?
> > >
> > > Exactly. Even though the drive itself may not be powered off, by
> > > putting it into runtime suspend we gain the ability to suspend the
> > > ancestor devices.
> >
> > OK, so what about using a PM QoS-based approach as described (in general
> > terms) in this message in the "USB ports power off" thread:
> >
> > http://marc.info/?l=linux-pm&m=134831537224566&w=4
>
> I'm not entirely sure. It may well work out better in this case than
> in the USB ports case.
>
> For the ZPODD stuff, the userspace control amounts to a single flag
> ("do not allow zero-power") which can easily be represented as a QoS
> constraint.
>
> For the USB ports, the situation is more complicated. The decision
> about whether or not to power-off a port depends not just on the port
> itself but also on the device plugged into the port, and there's no
> direct relation between the two in the sysfs device tree. (That could
> be fixed perhaps by adding symbolic links between them.)
That doesn't seem to be a big obstacle as far as PM QoS is concerned, though.
The trick may be to add PM QoS constraints not for the device itself, but
directly for the port it is connected to (it always is possible to add a
constraint for the device too, but that simply may be unnecessary). Then,
whoever decides whether or not to power off the port would only have to
look at the effective PM QoS requirement bits of the port.
> This should be discussed in the USB thread, though...
Sure.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/6] ZPODD patches
On Saturday, September 22, 2012, Alan Stern wrote: > On Sat, 22 Sep 2012, Rafael J. Wysocki wrote: > > > > There are sd devices with removable media. > > > > OK. Does the SCSI layer distinguish them from devices without removable > > media? > > Yes, it does. struct scsi_device has a .removable member, and the > Removable flag is part of the response data to the INQUIRY command > (which belongs to the primary command set common to all SCSI devices). > > > > > User space has an interface to disable runtime PM of any device and it > > > > looks > > > > like that interface should be sufficient to disable the feature in > > > > question. > > > > Why do you think the new interface is needed? > > > > > > Because this is not equivalent to doing no runtime PM at all. SCSI > > > now defines some powersaving states which do not involve powering > > > down and thus losing state. > > > > I see. So the sr's runtime suspend may be useful even without the power-off > > feature, right? > > Exactly. Even though the drive itself may not be powered off, by > putting it into runtime suspend we gain the ability to suspend the > ancestor devices. OK, so what about using a PM QoS-based approach as described (in general terms) in this message in the "USB ports power off" thread: http://marc.info/?l=linux-pm&m=134831537224566&w=4 Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/6] ZPODD patches
On Saturday, September 22, 2012, Oliver Neukum wrote: > On Friday 21 September 2012 23:18:27 Rafael J. Wysocki wrote: > > > Now, James says he doesn't like the way ready_to_power_off is used. Sure > > enough, it is totally irrelevant to the majority of SCSI devices. It > > actually > > is totally irrelevant to everything in the SCSI subsystem except for the sr > > driver and libata. So I wonder if you have considered any alternative > > way to address the use case at hand? > > Strictly speaking, USB on very modern systems could use it, but doesn't > in the current implementation. OK Still, why does the flag have to be in struct scsi_device for this purpose? > > That sounds reasonable enough, but the role of the powered_off and > > need_eject flags could be explained a bit better. In particular, it would > > I think need_eject needs to be renamed. Something like "media_change_detected" > > > be nice to have explained why they have to be present in struct scsi_device, > > because they don't seem to be particularly useful for many SCSI devices > > that aren't CD drives (the need_eject one in particular). > > There are sd devices with removable media. OK. Does the SCSI layer distinguish them from devices without removable media? > > User space has an interface to disable runtime PM of any device and it looks > > like that interface should be sufficient to disable the feature in question. > > Why do you think the new interface is needed? > > Because this is not equivalent to doing no runtime PM at all. SCSI > now defines some powersaving states which do not involve powering > down and thus losing state. I see. So the sr's runtime suspend may be useful even without the power-off feature, right? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/6] ZPODD patches
On Friday, September 21, 2012, Aaron Lu wrote: > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote: > > On Wednesday, September 19, 2012, Aaron Lu wrote: > > > Thanks Rafael, and if there is any question/problem, > > > please kindly let me know. > > > > Well, unfortunately my initial review indicates that the patchset is not > > quite ready to go upstream yet. > > > > I'll send comments in replies to the individual patches, but overall I can > > say that at this stage of development, when I look at the patches, it should > > be clear to me not only what is being changed, but _why_ it is being changed > > in the first place and, secondly, why it is being changed in this particular > > way. It's far from that, though. > > I'm adding zero power support for optical disk drive(ZPODD), which is > made possible with the newly defined device attention(DA) pin introduced > in SATA 3.1 spec. > > The idea here is to use runtime pm to achieve this, so I basically did 2 > things: > 1 Add runtime pm support for ODD; > 2 Add power off support for ODD after it is runtime suspended. > > Patch 2 is runtime pm support for ODD, the reason it is done this way is > discussed here: > http://www.spinics.net/lists/linux-scsi/msg61551.html Why isn't it explained in the patch changelog, then? People should be able to learn why things are done the way they are done from git logs. > The basic idea is, the ODD will be runtime suspended as long as there is > nobody using it, that is, no programs opening the block device. > > The ODD will be polled periodically, so it will be runtime resumed > before checking if there is any events pending and suspended when done. OK. So what happens if we power off the drive via runtime PM. Does it it really make sense to resumie it through polling in that case? > The only exception is, if we found a disc is just inserted, we will not > idle it immediately at the end of the poll, reason explained in another > mail. > > This is the rational I wrote patch 2, and patch 1 is used by patch 2. > > Patch 3 is adding power off support for ODD after it is runtime > suspended, the condition is specified in section 15: > ftp://ftp.seagate.com/sff/INF-8090.PDF > > That is, for tray type ODD: no media inside and door closed; for slot > type ODD: no media inside. > > The is the reason sr_suspend is written, for non-ZPODD capable devices, > it does nothing; for ZPODD devices, it will check the above condition to > see if it is ready to be powered off. The ready_to_power_off flag will be > used by ATA layer to decide if power can be removed. Now, James says he doesn't like the way ready_to_power_off is used. Sure enough, it is totally irrelevant to the majority of SCSI devices. It actually is totally irrelevant to everything in the SCSI subsystem except for the sr driver and libata. So I wonder if you have considered any alternative way to address the use case at hand? > When in powered off state, if user presses the eject button or insert a > disc, an ACPI event will be generated and our acpi wake handler will > pm_runtime_resume the ODD. And if it is a tray type ODD, its tray should > be ejected(need_eject flag) after powered on. This is patch 3. That sounds reasonable enough, but the role of the powered_off and need_eject flags could be explained a bit better. In particular, it would be nice to have explained why they have to be present in struct scsi_device, because they don't seem to be particularly useful for many SCSI devices that aren't CD drives (the need_eject one in particular). > And patch 4-6 introduces a new sysfs file may_power_off to give user a > chance to disable the power off of the device due to various reasons > like the ODD claims support of device attention while actually it is > broken. User space has an interface to disable runtime PM of any device and it looks like that interface should be sufficient to disable the feature in question. Why do you think the new interface is needed? > I hope it makes more sense to you now. Well, thanks for the explanation. :-) Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)
On Friday, September 21, 2012, Aaron Lu wrote:
> On Fri, Sep 21, 2012 at 12:07:23AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 12, 2012, Aaron Lu wrote:
> > > static struct scsi_driver sr_template = {
> > > .owner = THIS_MODULE,
> > > @@ -87,6 +89,8 @@ static struct scsi_driver sr_template = {
> > > .name = "sr",
> > > .probe = sr_probe,
> > > .remove = sr_remove,
> > > + .suspend= sr_suspend,
> > > + .resume = sr_resume,
> > > },
> > > .done = sr_done,
> > > };
> > > @@ -172,6 +176,52 @@ static void scsi_cd_put(struct scsi_cd *cd)
> > > mutex_unlock(&sr_ref_mutex);
> > > }
> >
> > Besides, I need some help to understand how this is supposed to work.
> >
> > Do I think correctly that sr_suspend(), for example, will be run by the
> > SCSI bus type layer in case of a CD device runtime suspend? However,
>
> Yes.
>
> > won't this routine be used during system suspend as well and won't it cause
> > problems to happen if so?
>
> On system suspend, nothing needs to be done.
> I'll add the following code in next version.
>
> if (!PMSG_IS_AUTO(msg))
> return 0;
Please don't. The pm_message_t thing is obsolete and shoulnd't really be
used by device drivers. I know that ATA relies on it internally, but that's
just something that needs to be changed at one point.
Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct
dev_pm_ops eventually and your change is kind of going in the opposite
direction. I don't know how much effort the migration is going to take,
though, so perhaps we can just make this change first.
On a slightly related note, suppose that we have an "enabled" bit in flags
in struct acpi_device_power_state and suppose that we have a helper
function pm_platform_power_off_allowed(dev, bool), such that if the driver
(or subsystem) does pm_platform_power_off_allowed(dev, false) and the
platform is ACPI, the "enabled" bit for the D3cold state will be cleared.
Then, the ACPI device PM routines will never use D3cold as the device
power state.
In that case, it seems, you won't need the "ready_to_power_off" flag in struct
scsi_device any more.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Friday, September 21, 2012, Aaron Lu wrote:
> On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 12, 2012, Aaron Lu wrote:
> > > Place the ODD into runtime suspend state as soon as there is nobody
> > > using it.
> >
> > OK, so how is ODD related to the sr driver?
>
> As Alan has explained, ODD(optical disk drive) is driven by scsi
> sr driver.
OK, but what about writing "ODD (Optical Disk Drive)" in the changelog?
People reading git logs may not know all of the hardware acronyms and the
"0" message doesn't go into the git log. :-)
> > > The only exception is, if we just find that a new medium is
> > > inserted, we wait for the next events checking to idle it.
> >
> > What exactly do you mean by "to idle it"?
>
> I mean to put its usage count so that its idle callback will kick in.
So I'd just write that directly in the changelog.
> > Does this patch have any functional effect without the following patches?
>
> Yes, this one alone takes care of ODD's runtime pm
I suppose you mean the runtime PM status and usage counter? I.e. the "software
state"?
> while the following
> patches take care of removing its power after it's runtime suspended.
> But it doesn't have any real benefit without the following patches.
Please put that information into the changelog too.
> > > Based on ideas of Alan Stern and Oliver Neukum.
> > >
> > > Signed-off-by: Aaron Lu
> > > ---
> > > drivers/scsi/sr.c | 29 +
> > > 1 file changed, 25 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > index 5fc97d2..7a8222f 100644
> > > --- a/drivers/scsi/sr.c
> > > +++ b/drivers/scsi/sr.c
> > > @@ -45,6 +45,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > >
> > > #include
> > > @@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct
> > > gendisk *disk)
> > > kref_get(&cd->kref);
> > > if (scsi_device_get(cd->device))
> > > goto out_put;
> > > + if (scsi_autopm_get_device(cd->device))
> > > + goto out_pm;
> > > goto out;
> >
> > Why don't you do
> >
> > > + if (!scsi_autopm_get_device(cd->device))
> > > + goto out;
> >
> > without the new label?
>
> I was just stupidly following the pattern.
> Thanks and I'll change this.
>
> >
> > >
> > > + out_pm:
> > > + scsi_device_put(cd->device);
> > > out_put:
> > > kref_put(&cd->kref, sr_kref_release);
> > > cd = NULL;
> > > @@ -163,6 +168,7 @@ static void scsi_cd_put(struct scsi_cd *cd)
> > > mutex_lock(&sr_ref_mutex);
> > > kref_put(&cd->kref, sr_kref_release);
> > > scsi_device_put(sdev);
> > > + scsi_autopm_put_device(sdev);
> > > mutex_unlock(&sr_ref_mutex);
> > > }
> > >
> > > @@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct
> > > cdrom_device_info *cdi,
> > > unsigned int clearing, int slot)
> > > {
> > > struct scsi_cd *cd = cdi->handle;
> > > - bool last_present;
> > > + bool last_present = cd->media_present;
> > > struct scsi_sense_hdr sshdr;
> > > unsigned int events;
> > > int ret;
> > > @@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct
> > > cdrom_device_info *cdi,
> > > if (CDSL_CURRENT != slot)
> > > return 0;
> > >
> > > + scsi_autopm_get_device(cd->device);
> > > +
> > > events = sr_get_events(cd->device);
> > > cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
> > >
> > > @@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct
> > > cdrom_device_info *cdi,
> > > }
> > >
> > > if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
> > > - return events;
> > > + goto out;
> > > do_tur:
> > > /* let's see whether the media is there with TUR */
> > > - last_present = cd->media_present;
> > > ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > >
> > > /*
> > > @@ -270,7 +277,7 @@ do_tur:
> > >
Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)
On Wednesday, September 12, 2012, Aaron Lu wrote:
> When ODD is runtime suspended, we will check if it is OK to remove
> its power:
> 1 For tray type, no medium inside and tray closed;
> 2 For slot type, no medium inside.
> And if yes, we will set the ready_to_power_off flag as an indication to
> ATA layer that it is safe to place this device into ACPI D3 cold power
> state.
>
> And when it is powered off, we will set the powered_off flag so that the
> periodically running check_events will not bother this device by simply
> return.
Well, so I agree with James that the PM flags in struct scsi_device are
kind of odd and it would be better to put them somewhere else.
I'd probably prefer them to sit in the ACPI layer with some helper functions
allowing the sr driver to manipulate them, but that depends on the resulting
code complexity.
> Signed-off-by: Aaron Lu
> ---
> drivers/ata/libata-acpi.c | 27 +++
> drivers/scsi/sr.c | 53
> ++
> drivers/scsi/sr.h | 1 +
> drivers/scsi/sr_ioctl.c| 7 +-
> include/scsi/scsi_device.h | 3 +++
> 5 files changed, 81 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 902b5a4..9aca057 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -854,7 +854,7 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t
> state)
> {
> struct ata_device *dev;
> acpi_handle handle;
> - int acpi_state;
> + int acpi_state, ret;
>
> /* channel first and then drives for power on and vica versa
> for power off */
> @@ -869,17 +869,24 @@ void ata_acpi_set_state(struct ata_port *ap,
> pm_message_t state)
>
> if (state.event != PM_EVENT_ON) {
> acpi_state = acpi_pm_device_sleep_state(
> - &dev->sdev->sdev_gendev, NULL, ACPI_STATE_D3);
> - if (acpi_state > 0)
> - acpi_bus_set_power(handle, acpi_state);
> - /* TBD: need to check if it's runtime pm request */
> - acpi_pm_device_run_wake(
> - &dev->sdev->sdev_gendev, true);
> + &dev->sdev->sdev_gendev, NULL,
> + dev->sdev->ready_to_power_off ?
> + ACPI_STATE_D3 : ACPI_STATE_D3_HOT);
> + if (acpi_state > 0) {
> + ret = acpi_bus_set_power(handle, acpi_state);
> + if (!ret && acpi_state == ACPI_STATE_D3)
> + dev->sdev->powered_off = 1;
> +
> + /* TODO: check if it's runtime pm request */
> + acpi_pm_device_run_wake(
> + &dev->sdev->sdev_gendev, true);
> + }
> } else {
> /* Ditto */
> acpi_pm_device_run_wake(
> &dev->sdev->sdev_gendev, false);
> acpi_bus_set_power(handle, ACPI_STATE_D0);
> + dev->sdev->powered_off = 0;
> }
> }
>
> @@ -985,8 +992,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32
> event, void *context)
> struct ata_device *ata_dev = context;
>
> if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
> - pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
> - scsi_autopm_get_device(ata_dev->sdev);
> + ata_dev->sdev->powered_off) {
> + ata_dev->sdev->need_eject = 1;
> + pm_runtime_resume(&ata_dev->sdev->sdev_gendev);
> + }
> }
>
> static void ata_acpi_add_pm_notifier(struct ata_device *dev)
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 7a8222f..ef72682 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -80,6 +80,8 @@ static DEFINE_MUTEX(sr_mutex);
> static int sr_probe(struct device *);
> static int sr_remove(struct device *);
> static int sr_done(struct scsi_cmnd *);
> +static int sr_suspend(struct device *, pm_message_t msg);
> +static int sr_resume(struct device *);
>
> static struct scsi_driver sr_template = {
> .owner = THIS_MODULE,
> @@ -87,6 +89,8 @@ static struct scsi_driver sr_template = {
> .name = "sr",
> .probe = sr_probe,
> .remove = sr_remove,
> + .suspend= sr_suspend,
> + .resume = sr_resume,
> },
> .done = sr_done,
> };
> @@ -172,6 +176,52 @@ static void scsi_cd_put(struct scsi_cd *cd)
> mutex_unlock(&sr_ref_mutex);
> }
Besides, I need some help to understand how this is supposed to work.
Do I think correctly that sr_suspend(), for ex
Re: [PATCH v7 0/6] ZPODD patches
On Wednesday, September 19, 2012, James Bottomley wrote: > On Wed, 2012-09-19 at 14:50 +0200, Rafael J. Wysocki wrote: > > On Wednesday, September 19, 2012, James Bottomley wrote: > > > On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote: > > > > Hi James, > > > > > > > > May I know if this patchset will enter v3.7? > > > > > > Sigh, well, I was hoping to persuade the PM people to sort this out > > > first. > > > > > > The first observation is that all this looks to be too specific. ZPO > > > may be ACPI specific, but the property it abstracts: whether the > > > particular device is powered off or not is generic and probably should > > > be known at the generic PM level. Nothing actually really cares about > > > how we power off the device until you get all the way down to the ACPI > > > controller. > > > > > > I think we could do this with a couple of flags sitting inside struct > > > device itself: one for pm state and capabilities defined at a generic > > > level and one for device specific pm state. The latter would be for > > > things like the door lock information which is very specific to CDs > > > (although not specific to SCSI CDs). Alternatively, even if we can't > > > use these capabilities at the generic pm level, we still need an > > > internal state set of flags because power state stuff traverses the > > > stack and struct device is the only universal object in that stack. > > > > > > So I definitely think all of the sdev flags should become either generic > > > or specific flags in device. > > > > Well, the problem is that it is kind of irrelevant to the core whether or > > not the given device can be powered off. Moreover, the actual meaning of > > what "power off" means depends on the platform (it may be an individual > > device > > state or a power domain state, for instance). Also, the set of available > > low-power states depends on the platform (or the bus type) and generally > > cannot be universally represented and there are low-power states that > > aren't "power off" per se, but still require the device state to be > > restored when putting it back into full power. > > So I don't insist on it being generic, but we do need somewhere to hang > the state. > > > We've discussed that for a few times and each time we've ended up agreeing > > that struct device is not the right place to store this information (for > > example, PCI stores it in struct pci_dev, USB has its own rules etc.). > > So, here's the problem this causes. In SCSI, lower level devices have > no access to the drivers (to which the upper layer structures are tied), > so we have no way to go from device/scsi device to the scsi_disk > structure say. This means that a lot of device specific PM stuff tends > to have flags in scsi_device just so we can get access to it. A flag in > device would allow us to carry the information farther (say to struct > cdrom for instance). > > > I'll have a look at the patchset again and see what can be done about this. I think I see what you mean. For example, the sr driver uses its "device" member to pass information to the libata layer, if I understand things correctly, and the libata layer cannot go back to its struct scsi_cd, right? First off, I agree with you that putting those PM fields into struct scsi_device is not the cleanest approach and it would be good to find some other place for them. However, I also don't think that struct device (or struct dev_pm_info embedded in it for that matter) is any better (those fields in there would make as little sense as they do in struct scsi_device). Now, the question is where to store them and I think there are a couple of places worth considering. For instance, there's the void *driver_data field in struct acpi_device that I bet is unused for the ACPI devices associated with ATA ones and in principle it might be set by libata to point to a PM data structure (that would require some "platform" helper functions for the sr, sd etc. drivers to access that structure). There also is the subsys_data field in struct dev_pm_info that might be used to point to some libata-specific PM data (although the question is which struct dev_pm_info to use in that case, the sdev's one, or the sdev->gendev's one?). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Wednesday, September 12, 2012, Aaron Lu wrote:
> Place the ODD into runtime suspend state as soon as there is nobody
> using it.
OK, so how is ODD related to the sr driver?
> The only exception is, if we just find that a new medium is
> inserted, we wait for the next events checking to idle it.
What exactly do you mean by "to idle it"?
Does this patch have any functional effect without the following patches?
> Based on ideas of Alan Stern and Oliver Neukum.
>
> Signed-off-by: Aaron Lu
> ---
> drivers/scsi/sr.c | 29 +
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5fc97d2..7a8222f 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -45,6 +45,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk
> *disk)
> kref_get(&cd->kref);
> if (scsi_device_get(cd->device))
> goto out_put;
> + if (scsi_autopm_get_device(cd->device))
> + goto out_pm;
> goto out;
Why don't you do
> + if (!scsi_autopm_get_device(cd->device))
> + goto out;
without the new label?
>
> + out_pm:
> + scsi_device_put(cd->device);
> out_put:
> kref_put(&cd->kref, sr_kref_release);
> cd = NULL;
> @@ -163,6 +168,7 @@ static void scsi_cd_put(struct scsi_cd *cd)
> mutex_lock(&sr_ref_mutex);
> kref_put(&cd->kref, sr_kref_release);
> scsi_device_put(sdev);
> + scsi_autopm_put_device(sdev);
> mutex_unlock(&sr_ref_mutex);
> }
>
> @@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct
> cdrom_device_info *cdi,
> unsigned int clearing, int slot)
> {
> struct scsi_cd *cd = cdi->handle;
> - bool last_present;
> + bool last_present = cd->media_present;
> struct scsi_sense_hdr sshdr;
> unsigned int events;
> int ret;
> @@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct
> cdrom_device_info *cdi,
> if (CDSL_CURRENT != slot)
> return 0;
>
> + scsi_autopm_get_device(cd->device);
> +
> events = sr_get_events(cd->device);
> cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
>
> @@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct
> cdrom_device_info *cdi,
> }
>
> if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
> - return events;
> + goto out;
> do_tur:
> /* let's see whether the media is there with TUR */
> - last_present = cd->media_present;
> ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
>
> /*
> @@ -270,7 +277,7 @@ do_tur:
> }
>
> if (cd->ignore_get_event)
> - return events;
> + goto out;
>
> /* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
> if (!cd->tur_changed) {
> @@ -287,6 +294,12 @@ do_tur:
> cd->tur_changed = false;
> cd->get_event_changed = false;
>
> +out:
> + if (cd->media_present && !last_present)
> + pm_runtime_put_noidle(&cd->device->sdev_gendev);
> + else
> + scsi_autopm_put_device(cd->device);
> +
This thing is asking for a comment.
It looks like you're kind of avoiding to call _idle() for the device, but why?
What might go wrong if pm_runtime_put() is used instead of the whole
conditional,
among other things?
> return events;
> }
>
> @@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
> dev_set_drvdata(dev, cd);
> disk->flags |= GENHD_FL_REMOVABLE;
> add_disk(disk);
> + disk_events_set_poll_msecs(disk, 5000);
Why do you need this and why is the poll interval universally suitable?
>
> sdev_printk(KERN_DEBUG, sdev,
> "Attached scsi CD-ROM %s\n", cd->cdi.name);
> +
> + /* enable runtime pm */
Not really. What it does is to enable the device to be suspended.
> + scsi_autopm_put_device(cd->device);
> +
> return 0;
>
> fail_put:
> @@ -965,6 +983,9 @@ static int sr_remove(struct device *dev)
> {
> struct scsi_cd *cd = dev_get_drvdata(dev);
>
> + /* disable runtime pm */
And that prevents the device from being suspended (which means that it's
going to be resumed at this point in case it was suspended before).
> + scsi_autopm_get_device(cd->device);
> +
> blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
> del_gendisk(cd->disk);
>
>
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/6] block: genhd: add an interface to set disk poll interval
Please add a changelog explaining who's going to use the new interface, in
addition to the original user of that code, and why it is exported.
Thanks,
Rafael
On Wednesday, September 12, 2012, Aaron Lu wrote:
> Signed-off-by: Aaron Lu
> ---
> block/genhd.c | 23 +--
> include/linux/genhd.h | 1 +
> 2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index cac7366..4244256 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1627,6 +1627,19 @@ static void disk_events_workfn(struct work_struct
> *work)
> kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
> }
>
> +int disk_events_set_poll_msecs(struct gendisk *disk, long intv)
> +{
> + if (intv < 0 && intv != -1)
> + return -EINVAL;
> +
> + disk_block_events(disk);
> + disk->ev->poll_msecs = intv;
> + __disk_unblock_events(disk, true);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(disk_events_set_poll_msecs);
> +
> /*
> * A disk events enabled device has the following sysfs nodes under
> * its /sys/block/X/ directory.
> @@ -1683,16 +1696,14 @@ static ssize_t disk_events_poll_msecs_store(struct
> device *dev,
> {
> struct gendisk *disk = dev_to_disk(dev);
> long intv;
> + int ret;
>
> if (!count || !sscanf(buf, "%ld", &intv))
> return -EINVAL;
>
> - if (intv < 0 && intv != -1)
> - return -EINVAL;
> -
> - disk_block_events(disk);
> - disk->ev->poll_msecs = intv;
> - __disk_unblock_events(disk, true);
> + ret = disk_events_set_poll_msecs(disk, intv);
> + if (ret)
> + return ret;
>
> return count;
> }
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 4f440b3..63409e5 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -423,6 +423,7 @@ extern void disk_block_events(struct gendisk *disk);
> extern void disk_unblock_events(struct gendisk *disk);
> extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
> extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int
> mask);
> +extern int disk_events_set_poll_msecs(struct gendisk *disk, long intv);
>
> /* drivers/char/random.c */
> extern void add_disk_randomness(struct gendisk *disk);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/6] ZPODD patches
On Wednesday, September 19, 2012, Aaron Lu wrote: > On 09/19/2012 08:50 PM, Rafael J. Wysocki wrote: > > On Wednesday, September 19, 2012, James Bottomley wrote: > >> On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote: > >>> Hi James, > >>> > >>> May I know if this patchset will enter v3.7? > >> > >> Sigh, well, I was hoping to persuade the PM people to sort this out > >> first. > >> > >> The first observation is that all this looks to be too specific. ZPO > >> may be ACPI specific, but the property it abstracts: whether the > >> particular device is powered off or not is generic and probably should > >> be known at the generic PM level. Nothing actually really cares about > >> how we power off the device until you get all the way down to the ACPI > >> controller. > >> > >> I think we could do this with a couple of flags sitting inside struct > >> device itself: one for pm state and capabilities defined at a generic > >> level and one for device specific pm state. The latter would be for > >> things like the door lock information which is very specific to CDs > >> (although not specific to SCSI CDs). Alternatively, even if we can't > >> use these capabilities at the generic pm level, we still need an > >> internal state set of flags because power state stuff traverses the > >> stack and struct device is the only universal object in that stack. > >> > >> So I definitely think all of the sdev flags should become either generic > >> or specific flags in device. > > > > Well, the problem is that it is kind of irrelevant to the core whether or > > not the given device can be powered off. Moreover, the actual meaning of > > what "power off" means depends on the platform (it may be an individual > > device > > state or a power domain state, for instance). Also, the set of available > > low-power states depends on the platform (or the bus type) and generally > > cannot be universally represented and there are low-power states that > > aren't "power off" per se, but still require the device state to be > > restored when putting it back into full power. > > > > We've discussed that for a few times and each time we've ended up agreeing > > that struct device is not the right place to store this information (for > > example, PCI stores it in struct pci_dev, USB has its own rules etc.). > > > > I'll have a look at the patchset again and see what can be done about this. > > Thanks Rafael, and if there is any question/problem, > please kindly let me know. Well, unfortunately my initial review indicates that the patchset is not quite ready to go upstream yet. I'll send comments in replies to the individual patches, but overall I can say that at this stage of development, when I look at the patches, it should be clear to me not only what is being changed, but _why_ it is being changed in the first place and, secondly, why it is being changed in this particular way. It's far from that, though. The first patch in the series doesn't even have a changelog. How the changelog of the second patch is related to it's actual contents is more than quite unclear to me. Etc. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/6] ZPODD patches
On Wednesday, September 19, 2012, James Bottomley wrote: > On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote: > > Hi James, > > > > May I know if this patchset will enter v3.7? > > Sigh, well, I was hoping to persuade the PM people to sort this out > first. > > The first observation is that all this looks to be too specific. ZPO > may be ACPI specific, but the property it abstracts: whether the > particular device is powered off or not is generic and probably should > be known at the generic PM level. Nothing actually really cares about > how we power off the device until you get all the way down to the ACPI > controller. > > I think we could do this with a couple of flags sitting inside struct > device itself: one for pm state and capabilities defined at a generic > level and one for device specific pm state. The latter would be for > things like the door lock information which is very specific to CDs > (although not specific to SCSI CDs). Alternatively, even if we can't > use these capabilities at the generic pm level, we still need an > internal state set of flags because power state stuff traverses the > stack and struct device is the only universal object in that stack. > > So I definitely think all of the sdev flags should become either generic > or specific flags in device. Well, the problem is that it is kind of irrelevant to the core whether or not the given device can be powered off. Moreover, the actual meaning of what "power off" means depends on the platform (it may be an individual device state or a power domain state, for instance). Also, the set of available low-power states depends on the platform (or the bus type) and generally cannot be universally represented and there are low-power states that aren't "power off" per se, but still require the device state to be restored when putting it back into full power. We've discussed that for a few times and each time we've ended up agreeing that struct device is not the right place to store this information (for example, PCI stores it in struct pci_dev, USB has its own rules etc.). I'll have a look at the patchset again and see what can be done about this. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new runtime scsi warnings in 2.6.24-rc6+git
On Friday, 4 of January 2008, Meelis Roos wrote: > Todays git gives the following warning during bootup on a Intel 845+PATA > PC (using libata to drive PATA): > > Driver 'sd' needs updating - please use bus_type methods > Driver 'sr' needs updating - please use bus_type methods They are due to commit 751bf4d7865e4ced406be93b04c7436d866d3684, AFAICS, and they don't mean anything wrong. Thanks, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 9674] New: Oops during rmmod'ing modeuls sdhci, sr_mod, ricoh_mmc, mmc_core
On Wednesday, 2 of January 2008, James Bottomley wrote: > > On Tue, 2008-01-01 at 18:10 -0800, Andrew Morton wrote: > > On Tue, 1 Jan 2008 14:55:45 -0800 (PST) [EMAIL PROTECTED] wrote: > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=9674 > > > > > >Summary: Oops during rmmod'ing modeuls sdhci, sr_mod, > > > ricoh_mmc, > > > mmc_core > > > > Guys, this is a very recent regression. Could you please take a look, see > > if it's due to mmc, block or scsi changes? > > There's not a lot of information to go on. The stack trace looks bogus, > so I guess the kernel is compiled without a frame pointer. The bug report has been updated with a stack trace from a kernel compiled with a frame pointer. Please have a look. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] 2.6.23-rc3 can't see sd partitions on Alpha
On Friday, 7 of December 2007, Bob Tracy wrote: > OK. Finally have this thing painted into a corner: git has identified > 6f37ac793d6ba7b35d338f791974166f67fdd9ba as the first bad commit. > > From "git bisect log", this corresponds to > > # bad: [6f37ac793d6ba7b35d338f791974166f67fdd9ba] Merge branch 'master' of > master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6 Something's gone wrong, as this commit doesn't modify code. > Here's the full log: > > git-bisect start > # good: [9aae299f7fd1888ea3a195cfe0edef17bb647415] Linux 2.6.24-rc2 > git-bisect good 9aae299f7fd1888ea3a195cfe0edef17bb647415 > # bad: [f05092637dc0d9a3f2249c9b283b973e6e96b7d2] Linux 2.6.24-rc3 > git-bisect bad f05092637dc0d9a3f2249c9b283b973e6e96b7d2 > # good: [e6a5c27f3b0fef72e528fc35e343af4b2db790ff] Merge branch 'for-linus' > of git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm > git-bisect good e6a5c27f3b0fef72e528fc35e343af4b2db790ff > # good: [42614fcde7bfdcbe43a7b17035c167dfebc354dd] vmstat: fix section > mismatch warning > git-bisect good 42614fcde7bfdcbe43a7b17035c167dfebc354dd > # bad: [a052f4473603765eb6b4c19754689977601dc1d1] Merge > git://git.kernel.org/pub/scm/linux/kernel/git/sam/x86 > git-bisect bad a052f4473603765eb6b4c19754689977601dc1d1 > # good: [d8e5219f9f5ca7518eb820db9f3d287a1d46fcf5] CRISv10 improve and bugfix > fasttimer > git-bisect good d8e5219f9f5ca7518eb820db9f3d287a1d46fcf5 > # good: [d90bf5a976793edfa88d3bb2393f0231eb8ce1e5] [NET]: rt_check_expire() > can take a long time, add a cond_resched() > git-bisect good d90bf5a976793edfa88d3bb2393f0231eb8ce1e5 > # good: [2a113281f5cd2febbab21a93c8943f8d3eece4d3] kconfig: use $K64BIT to > set 64BIT with all*config targets > git-bisect good 2a113281f5cd2febbab21a93c8943f8d3eece4d3 > # good: [2e2cd8bad6e03ceea73495ee6d557044213d95de] CRISv10 memset library add > lineendings to asm > git-bisect good 2e2cd8bad6e03ceea73495ee6d557044213d95de > # bad: [6f37ac793d6ba7b35d338f791974166f67fdd9ba] Merge branch 'master' of > master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6 > git-bisect bad 6f37ac793d6ba7b35d338f791974166f67fdd9ba > # good: [2f1f53bdc6531696934f6ee7bbdfa2ab4f4f62a3] CRISv10 fasttimer: Scrap > INLINE and name timeval_cmp better > git-bisect good 2f1f53bdc6531696934f6ee7bbdfa2ab4f4f62a3 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] 2.6.23-rc3 can't see sd partitions on Alpha
On Friday, 30 of November 2007, Andrew Morton wrote: > On Sat, 01 Dec 2007 11:30:01 +1300 > Michael Cree <[EMAIL PROTECTED]> wrote: > > > Bob Tracy wrote: > > > Andrew Morton wrote: > > >> Could be something change in sysfs. Please double-check the config > > >> options, make sure that something important didn't get disabled. > > >> > > > Here's > > > hoping someone else is seeing this or can replicate it in the meantime. > > > > Snap. > > > > 2.6.24-rc2 works fine. 2.6.24-rc3 boots on Alpha but once /dev is > > populated no partitions of the scsi sub-system are seen. Looks like ide > > sub-system similarly affected. > > Rafael, I assume you have this regression in the list? Yes, http://bugzilla.kernel.org/show_bug.cgi?id=9457 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 2.6.23-rc9 boot failure (megaraid?)
On Tuesday, 2 October 2007 20:46, Burton Windle wrote: > On Tue, 2 Oct 2007, Adrian Bunk wrote: > > > Cc's added, the complete bug report is at > > http://lkml.org/lkml/2007/10/2/243 > > > > On Tue, Oct 02, 2007 at 12:48:26PM -0400, Burton Windle wrote: > >> 2.6.23-rc9 fails to boot for me; 2.6.22.9 works fine. > >> > >> System is a Dell Poweredge with PERC 2/DC with RAID1 volume. > >> ... > > > > Thanks for your report. > > > > Does reverting the commit below fix the problem? > > > > cu > > Adrian > > > > > > commit 3f6270ef76f2ce5c134615a470685d6c2a66c07e > > Author: FUJITA Tomonori <[EMAIL PROTECTED]> > > Date: Mon May 14 20:17:27 2007 +0900 > > > > Confirmed; reverting the above (snipped) patch does fix the issue. I've created a bugzilla entry for your report at: http://bugzilla.kernel.org/show_bug.cgi?id=9113 Please add a summary of your observations in there. Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
