* Re: [PATCH 1/2] libata: initialize link speed from sstatus if online.
From: Tejun Heo @ 2017-10-21 16:48 UTC (permalink / raw)
To: David Milburn; +Cc: linux-ide
In-Reply-To: <1508437836-31649-2-git-send-email-dmilburn@redhat.com>
Hello, David.
On Thu, Oct 19, 2017 at 01:30:35PM -0500, David Milburn wrote:
> @@ -3002,16 +3002,16 @@ int ata_bus_probe(struct ata_port *ap)
> */
> static void sata_print_link_status(struct ata_link *link)
> {
> - u32 sstatus, scontrol, tmp;
> + u32 sstatus, scontrol;
>
> if (sata_scr_read(link, SCR_STATUS, &sstatus))
> return;
> sata_scr_read(link, SCR_CONTROL, &scontrol);
>
> if (ata_phys_link_online(link)) {
> - tmp = (sstatus >> 4) & 0xf;
> + link->sata_spd = (sstatus >> 4) & 0xf;
> ata_link_info(link, "SATA link up %s (SStatus %X SControl %X)\n",
> - sata_spd_string(tmp), sstatus, scontrol);
> + sata_spd_string(link->sata_spd), sstatus, scontrol);
> } else {
> ata_link_info(link, "SATA link down (SStatus %X SControl %X)\n",
> sstatus, scontrol);
I don't think it makes sense to update a link field from a
print_status function.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] ata: mark expected switch fall-throughs
From: Tejun Heo @ 2017-10-21 16:29 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Hans de Goede, Bartlomiej Zolnierkiewicz, linux-ide, linux-kernel
In-Reply-To: <20171012191916.GA2974@embeddedor.com>
On Thu, Oct 12, 2017 at 02:19:16PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> In cases where a "drop through" comment was already in place, I replaced
> it with a proper "fall through" comment, which is what GCC is expecting
> to find.
>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Applied to libata/for-4.15.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] ata: sata_mv: remove a redundant assignment to pointer ehi
From: Tejun Heo @ 2017-10-21 16:25 UTC (permalink / raw)
To: Colin King; +Cc: linux-ide, kernel-janitors, linux-kernel
In-Reply-To: <20171016110011.11048-1-colin.king@canonical.com>
On Mon, Oct 16, 2017 at 12:00:11PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer ehi is being assigned to a value that is never read
> and is redundant. Clean up the code and move the ehi declaration
> and initialization to the code block where it is used. Cleans up
> clang warning: Value stored to 'ehi' is never read
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied to libata/for-4.15.
Thanks.
--
tejun
^ permalink raw reply
* Re: BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
From: Tejun Heo @ 2017-10-21 16:23 UTC (permalink / raw)
To: Yu Chen; +Cc: linux-ide, Hannes Reinecke
In-Reply-To: <CADjb_WS86DytQ1FFMG-ucv2Pr282hacGzqpeW0_aHjON=_j5+g@mail.gmail.com>
(cc'ing Hannes and quoting the whole message)
Hannes, this is caused by 8a97712e5314aefe16b3ffb4583a34deaa49de04
("scsi: make 'state' device attribute pollable").
Thanks.
On Wed, Oct 18, 2017 at 04:56:05PM +0800, Yu Chen wrote:
> I saw the following backtrace when running some benchmarks, it might
> not be critical, but it is even better we can get rid of this warning : )
>
>
> [ 318.808335] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:238
> [ 318.818779] in_atomic(): 1, irqs_disabled(): 1, pid: 1144, name: scsi_eh_7
> [ 318.826248] CPU: 5 PID: 1144 Comm: scsi_eh_7 Tainted: G O
> 4.14.0-rc4-00128-g30e9e23 #2
> [ 318.835882] Call Trace:
> [ 318.838973] dump_stack+0x63/0x86
> [ 318.842882] ___might_sleep+0xf1/0x110
> [ 318.847214] __might_sleep+0x4a/0x80
> [ 318.851368] mutex_lock+0x20/0x50
> [ 318.855247] kernfs_find_and_get_ns+0x23/0x60
> [ 318.860158] sysfs_notify+0x77/0x90
> [ 318.864237] scsi_device_set_state+0x63/0x150
> [ 318.869180] ata_scsi_offline_dev+0x1c/0x30 [libata]
> [ 318.874696] ata_eh_detach_dev+0x3b/0xb0 [libata]
> [ 318.879929] ata_eh_schedule_probe+0x59/0x1c0 [libata]
> [ 318.885590] ata_eh_recover+0x108/0x12d0 [libata]
> [ 318.890807] ? ahci_pmp_attach+0x70/0x70 [libahci]
> [ 318.896105] ? ahci_do_hardreset+0x110/0x110 [libahci]
> [ 318.901744] ? ahci_do_softreset+0x210/0x210 [libahci]
> [ 318.907371] ? ata_phys_link_offline+0x30/0x30 [libata]
> [ 318.913123] ? ata_eh_report+0x34f/0x850 [libata]
> [ 318.918345] sata_pmp_eh_recover+0xb7/0xa00 [libata]
> [ 318.923818] ? ata_eh_report+0x6e6/0x850 [libata]
> [ 318.929028] sata_pmp_error_handler+0x22/0x30 [libata]
> [ 318.934631] ahci_error_handler+0x1d/0x70 [libahci]
> [ 318.939968] ata_scsi_port_error_handler+0x430/0x720 [libata]
> [ 318.946167] ? ata_scsi_cmd_error_handler+0xe9/0x140 [libata]
> [ 318.952368] ata_scsi_error+0x86/0xb0 [libata]
> [ 318.957260] ? scsi_error_handler+0x38/0x5d0
> [ 318.961965] scsi_error_handler+0xe7/0x5d0
> [ 318.966499] kthread+0x114/0x150
> [ 318.970155] ? scsi_eh_get_sense+0x280/0x280
> [ 318.974848] ? kthread_create_on_node+0x40/0x40
> [ 318.979798] ret_from_fork+0x25/0x30
> [ 318.983813] ata8: hard resetting link
>
> Thanks,
> Yu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
tejun
^ permalink raw reply
* Re: [PATCH v2 2/9] ata: ceva: Move sata port phy oob settings to device-tree
From: Tejun Heo @ 2017-10-21 15:19 UTC (permalink / raw)
To: Michal Simek
Cc: linux-kernel, monstr, Alexander Graf, Rob Herring,
Anurag Kumar Vulisha, linux-ide
In-Reply-To: <b9b0e17612ef2d1e842f178bbc47ae6bd36258dd.1503314240.git.michal.simek@xilinx.com>
On Mon, Aug 21, 2017 at 01:17:17PM +0200, Michal Simek wrote:
> From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>
> In SATA Speed negotiation happens with OOB(Out of Band) signals. These OOB
> signal timing values are configured through vendor specific registers in the
> SATA controller. These OOB timings depends on the generator and detector clock
> frequency, which varies from board to board (ex: ep108 and zc1751 has different
> clock frequencies).
> To avoid maintaing these OOB settings in the driver, it is better to move these
> settings to the device-tree node and read from the device-tree.
>
> This patch does the same.
>
> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Applied to libata/for-4.15.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 2/9] ata: ceva: Move sata port phy oob settings to device-tree
From: Michal Simek @ 2017-10-20 10:42 UTC (permalink / raw)
To: Michal Simek, linux-kernel, Tejun Heo
Cc: Alexander Graf, Rob Herring, Anurag Kumar Vulisha, linux-ide
In-Reply-To: <b9b0e17612ef2d1e842f178bbc47ae6bd36258dd.1503314240.git.michal.simek@xilinx.com>
[-- Attachment #1.1: Type: text/plain, Size: 1109 bytes --]
Hi Tejun,
On 21.8.2017 13:17, Michal Simek wrote:
> From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>
> In SATA Speed negotiation happens with OOB(Out of Band) signals. These OOB
> signal timing values are configured through vendor specific registers in the
> SATA controller. These OOB timings depends on the generator and detector clock
> frequency, which varies from board to board (ex: ep108 and zc1751 has different
> clock frequencies).
> To avoid maintaing these OOB settings in the driver, it is better to move these
> settings to the device-tree node and read from the device-tree.
>
> This patch does the same.
>
> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Can you please look at these patches?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH V8 5/5] libata: Align DMA buffer todma_get_cache_alignment()
From: 陈华才 @ 2017-10-20 4:25 UTC (permalink / raw)
To: Matt Redfearn, Tejun Heo
Cc: Christoph Hellwig, Marek Szyprowski, Robin Murphy, AndrewMorton,
Fuxin Zhang, linux-kernel, Ralf Baechle, JamesHogan, linux-mips,
James E . J .Bottomley, Martin K . Petersen, linux-scsi,
linux-ide
In-Reply-To: <0ba3dc38-9020-1062-57de-0ada2cfd43a9@mips.com>
Hi, Matt,
I found that 4ee34ea3a12396f35b26d90a094c75db ("libata: Align ata_device's id on a cacheline") can resolve everything. Because the size of id[ATA_ID_WORDS] is already aligned and devslp_timing needn't to be aligned. So, In V9 of this series I will drop this patch. Why I had problems before? because I used linux-4.4.
Huacai
------------------ Original ------------------
From: "Matt Redfearn"<matt.redfearn@mips.com>;
Date: Thu, Oct 19, 2017 03:52 PM
To: "Tejun Heo"<tj@kernel.org>; "Huacai Chen"<chenhc@lemote.com>;
Cc: "Christoph Hellwig"<hch@lst.de>; "Marek Szyprowski"<m.szyprowski@samsung.com>; "Robin Murphy"<robin.murphy@arm.com>; "AndrewMorton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-kernel"<linux-kernel@vger.kernel.org>; "Ralf Baechle"<ralf@linux-mips.org>; "JamesHogan"<james.hogan@imgtec.com>; "linux-mips"<linux-mips@linux-mips.org>; "James E . J .Bottomley"<jejb@linux.vnet.ibm.com>; "Martin K . Petersen"<martin.petersen@oracle.com>; "linux-scsi"<linux-scsi@vger.kernel.org>; "linux-ide"<linux-ide@vger.kernel.org>; "stable"<stable@vger.kernel.org>;
Subject: Re: [PATCH V8 5/5] libata: Align DMA buffer todma_get_cache_alignment()
On 18/10/17 14:03, Tejun Heo wrote:
> On Tue, Oct 17, 2017 at 04:05:42PM +0800, Huacai Chen wrote:
>> In non-coherent DMA mode, kernel uses cache flushing operations to
>> maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer
>> should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer
>> and a kernel structure share a same cache line, and if the kernel
>> structure has dirty data, cache_invalidate (no writeback) will cause
>> data corruption.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> ---
>> drivers/ata/libata-core.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index ee4c1ec..e134955 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev)
>> unsigned int ata_do_dev_read_id(struct ata_device *dev,
>> struct ata_taskfile *tf, u16 *id)
>> {
>> - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE,
>> - id, sizeof(id[0]) * ATA_ID_WORDS, 0);
>> + u16 *devid;
>> + int res, size = sizeof(u16) * ATA_ID_WORDS;
>> +
>> + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev)))
>> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0);
>> + else {
>> + devid = kmalloc(size, GFP_KERNEL);
>> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0);
>> + memcpy(id, devid, size);
>> + kfree(devid);
>> + }
>> +
>> + return res;
> Hmm... I think it'd be a lot better to ensure that the buffers are
> aligned properly to begin with. There are only two buffers which are
> used for id reading - ata_port->sector_buf and ata_device->id. Both
> are embedded arrays but making them separately allocated aligned
> buffers shouldn't be difficult.
>
> Thanks.
FWIW, I agree that the buffers used for DMA should be split out from the
structure. We ran into this problem on MIPS last year,
4ee34ea3a12396f35b26d90a094c75db95080baa ("libata: Align ata_device's id
on a cacheline") partially fixed it, but likely should have also
cacheline aligned the following devslp_timing in the struct such that we
guarantee that members of the struct not used for DMA do not share the
same cacheline as the DMA buffer. Not having this means that
architectures, such as MIPS, which in some cases have to perform manual
invalidation of DMA buffer can clobber valid adjacent data if it is in
the same cacheline.
Thanks,
Matt
^ permalink raw reply
* (unknown),
From: dengx @ 2017-10-20 3:19 UTC (permalink / raw)
To: linux-ide
[-- Attachment #1: 94585642821.zip --]
[-- Type: application/zip, Size: 43584 bytes --]
^ permalink raw reply
* Re: [PATCH V8 4/5] libsas: Align SMP req/resp to dma_get_cache_alignment()
From: kbuild test robot @ 2017-10-20 0:04 UTC (permalink / raw)
Cc: kbuild-all, Christoph Hellwig, Marek Szyprowski, Robin Murphy,
Andrew Morton, Fuxin Zhang, linux-kernel, Ralf Baechle,
James Hogan, linux-mips, James E . J . Bottomley,
Martin K . Petersen, linux-scsi, Tejun Heo, linux-ide,
Huacai Chen, stable
In-Reply-To: <1508227542-13165-4-git-send-email-chenhc@lemote.com>
[-- Attachment #1: Type: text/plain, Size: 9547 bytes --]
Hi Huacai,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc5 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Huacai-Chen/dma-mapping-Rework-dma_get_cache_alignment/20171020-050317
config: um-allyesconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=um
All errors (new ones prefixed by >>):
drivers/scsi/libsas/sas_expander.c: In function 'sas_ex_phy_discover':
>> drivers/scsi/libsas/sas_expander.c:410:10: error: implicit declaration of function 'dma_get_cache_alignment' [-Werror=implicit-function-declaration]
align = dma_get_cache_alignment(&dev->phy->dev);
^~~~~~~~~~~~~~~~~~~~~~~
Cyclomatic Complexity 5 include/linux/compiler.h:__write_once_size
Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:atomic_set
Cyclomatic Complexity 2 arch/x86/include/asm/bitops.h:set_bit
Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:constant_test_bit
Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:fls64
Cyclomatic Complexity 1 arch/x86/include/uapi/asm/swab.h:__arch_swab64
Cyclomatic Complexity 1 include/uapi/linux/swab.h:__fswab16
Cyclomatic Complexity 1 include/uapi/linux/swab.h:__fswab64
Cyclomatic Complexity 1 include/linux/log2.h:__ilog2_u64
Cyclomatic Complexity 1 include/linux/list.h:INIT_LIST_HEAD
Cyclomatic Complexity 2 include/linux/list.h:__list_add
Cyclomatic Complexity 1 include/linux/list.h:list_add_tail
Cyclomatic Complexity 1 include/linux/list.h:__list_del
Cyclomatic Complexity 2 include/linux/list.h:__list_del_entry
Cyclomatic Complexity 1 include/linux/list.h:list_del
Cyclomatic Complexity 1 include/asm-generic/getorder.h:__get_order
Cyclomatic Complexity 1 include/linux/spinlock.h:spinlock_check
Cyclomatic Complexity 1 include/linux/spinlock.h:spin_lock_irq
Cyclomatic Complexity 1 include/linux/spinlock.h:spin_unlock_irq
Cyclomatic Complexity 1 include/linux/spinlock.h:spin_unlock_irqrestore
Cyclomatic Complexity 1 include/linux/refcount.h:refcount_set
Cyclomatic Complexity 28 include/linux/slab.h:kmalloc_index
Cyclomatic Complexity 68 include/linux/slab.h:kmalloc_large
Cyclomatic Complexity 5 include/linux/slab.h:kmalloc
Cyclomatic Complexity 1 include/linux/slab.h:kzalloc
Cyclomatic Complexity 1 include/linux/kref.h:kref_init
Cyclomatic Complexity 1 include/linux/kref.h:kref_get
Cyclomatic Complexity 2 include/linux/kref.h:kref_put
Cyclomatic Complexity 1 include/scsi/scsi.h:scsi_to_u32
Cyclomatic Complexity 1 include/scsi/sas_ata.h:dev_is_sata
Cyclomatic Complexity 5 drivers/scsi/libsas/sas_internal.h:sas_fill_in_rphy
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_internal.h:sas_add_parent_port
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_internal.h:sas_alloc_device
Cyclomatic Complexity 1 drivers/scsi/libsas/sas_internal.h:sas_put_device
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_expander.c:alloc_smp_req
Cyclomatic Complexity 1 drivers/scsi/libsas/sas_expander.c:alloc_smp_resp
Cyclomatic Complexity 5 drivers/scsi/libsas/sas_expander.c:sas_route_char
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:to_dev_type
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:dev_type_flutter
Cyclomatic Complexity 3 drivers/scsi/libsas/sas_expander.c:sas_print_parent_topology_bug
Cyclomatic Complexity 17 drivers/scsi/libsas/sas_expander.c:smp_execute_task_sg
Cyclomatic Complexity 1 drivers/scsi/libsas/sas_expander.c:smp_execute_task
Cyclomatic Complexity 21 drivers/scsi/libsas/sas_expander.c:sas_configure_present
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:sas_get_phy_discover
Cyclomatic Complexity 3 drivers/scsi/libsas/sas_expander.c:sas_get_phy_change_count
Cyclomatic Complexity 6 drivers/scsi/libsas/sas_expander.c:sas_find_bcast_phy
Cyclomatic Complexity 6 drivers/scsi/libsas/sas_expander.c:sas_get_ex_change_count
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_expander.c:smp_task_timedout
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_expander.c:smp_task_done
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:ex_assign_report_general
Cyclomatic Complexity 22 drivers/scsi/libsas/sas_expander.c:sas_check_eeds
Cyclomatic Complexity 23 drivers/scsi/libsas/sas_expander.c:sas_check_parent_topology
Cyclomatic Complexity 11 drivers/scsi/libsas/sas_expander.c:sas_configure_set
Cyclomatic Complexity 3 drivers/scsi/libsas/sas_expander.c:sas_configure_phy
Cyclomatic Complexity 11 drivers/scsi/libsas/sas_expander.c:sas_configure_parent
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_expander.c:sas_configure_routing
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_expander.c:sas_disable_routing
Cyclomatic Complexity 5 drivers/scsi/libsas/sas_expander.c:sas_find_sub_addr
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:sas_get_phy_attached_dev
Cyclomatic Complexity 35 drivers/scsi/libsas/sas_expander.c:sas_set_ex_phy
Cyclomatic Complexity 3 drivers/scsi/libsas/sas_expander.c:sas_ex_phy_discover_helper
Cyclomatic Complexity 10 drivers/scsi/libsas/sas_expander.c:sas_ex_general
Cyclomatic Complexity 3 drivers/scsi/libsas/sas_expander.c:ex_assign_manuf_info
Cyclomatic Complexity 7 drivers/scsi/libsas/sas_expander.c:sas_ex_manuf_info
Cyclomatic Complexity 6 drivers/scsi/libsas/sas_expander.c:sas_ex_get_linkrate
Cyclomatic Complexity 5 drivers/scsi/libsas/sas_expander.c:sas_ex_join_wide_port
Cyclomatic Complexity 10 drivers/scsi/libsas/sas_expander.c:sas_dev_present_in_domain
Cyclomatic Complexity 12 drivers/scsi/libsas/sas_expander.c:sas_ex_discover_end_dev
Cyclomatic Complexity 6 drivers/scsi/libsas/sas_expander.c:sas_unregister_ex_tree
Cyclomatic Complexity 12 drivers/scsi/libsas/sas_expander.c:sas_unregister_devs_sas_addr
Cyclomatic Complexity 10 drivers/scsi/libsas/sas_expander.c:sas_find_bcast_dev
Cyclomatic Complexity 5 drivers/scsi/libsas/sas_expander.c:sas_ex_to_ata
Cyclomatic Complexity 7 drivers/scsi/libsas/sas_expander.c:sas_ex_phy_discover
Cyclomatic Complexity 3 drivers/scsi/libsas/sas_expander.c:sas_expander_discover
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:sas_smp_phy_control
Cyclomatic Complexity 1 drivers/scsi/libsas/sas_expander.c:sas_ex_disable_phy
Cyclomatic Complexity 13 drivers/scsi/libsas/sas_expander.c:sas_check_ex_subtractive_boundary
Cyclomatic Complexity 7 drivers/scsi/libsas/sas_expander.c:sas_discover_expander
Cyclomatic Complexity 9 drivers/scsi/libsas/sas_expander.c:sas_ex_discover_expander
Cyclomatic Complexity 6 drivers/scsi/libsas/sas_expander.c:sas_ex_disable_port
Cyclomatic Complexity 14 drivers/scsi/libsas/sas_expander.c:sas_check_level_subtractive_boundary
Cyclomatic Complexity 40 drivers/scsi/libsas/sas_expander.c:sas_ex_discover_dev
Cyclomatic Complexity 7 drivers/scsi/libsas/sas_expander.c:sas_ex_discover_devices
Cyclomatic Complexity 8 drivers/scsi/libsas/sas_expander.c:sas_ex_level_discovery
Cyclomatic Complexity 2 drivers/scsi/libsas/sas_expander.c:sas_ex_bfs_disc
Cyclomatic Complexity 8 drivers/scsi/libsas/sas_expander.c:sas_discover_bfs_by_root_level
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:sas_discover_bfs_by_root
Cyclomatic Complexity 12 drivers/scsi/libsas/sas_expander.c:sas_discover_new
Cyclomatic Complexity 22 drivers/scsi/libsas/sas_expander.c:sas_rediscover_dev
Cyclomatic Complexity 9 drivers/scsi/libsas/sas_expander.c:sas_rediscover
Cyclomatic Complexity 5 drivers/scsi/libsas/sas_expander.c:sas_smp_get_phy_events
Cyclomatic Complexity 4 drivers/scsi/libsas/sas_expander.c:sas_discover_root_expander
Cyclomatic Complexity 5 drivers/scsi/libsas/sas_expander.c:sas_ex_revalidate_domain
Cyclomatic Complexity 7 drivers/scsi/libsas/sas_expander.c:sas_smp_handler
Cyclomatic Complexity 1 drivers/scsi/libsas/sas_expander.c:_GLOBAL__sub_I_65535_0_sas_ex_to_ata
cc1: some warnings being treated as errors
vim +/dma_get_cache_alignment +410 drivers/scsi/libsas/sas_expander.c
402
403 int sas_ex_phy_discover(struct domain_device *dev, int single)
404 {
405 struct expander_device *ex = &dev->ex_dev;
406 int res = 0, align;
407 u8 *disc_req;
408 u8 *disc_resp;
409
> 410 align = dma_get_cache_alignment(&dev->phy->dev);
411
412 disc_req = alloc_smp_req(DISCOVER_REQ_SIZE, align);
413 if (!disc_req)
414 return -ENOMEM;
415
416 disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE, align);
417 if (!disc_resp) {
418 kfree(disc_req);
419 return -ENOMEM;
420 }
421
422 disc_req[1] = SMP_DISCOVER;
423
424 if (0 <= single && single < ex->num_phys) {
425 res = sas_ex_phy_discover_helper(dev, disc_req, disc_resp, single);
426 } else {
427 int i;
428
429 for (i = 0; i < ex->num_phys; i++) {
430 res = sas_ex_phy_discover_helper(dev, disc_req,
431 disc_resp, i);
432 if (res)
433 goto out_err;
434 }
435 }
436 out_err:
437 kfree(disc_resp);
438 kfree(disc_req);
439 return res;
440 }
441
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19622 bytes --]
^ permalink raw reply
* [PATCH 2/2] Allow error handler to change speed only if link is online.
From: David Milburn @ 2017-10-19 18:30 UTC (permalink / raw)
To: linux-ide; +Cc: tj
In-Reply-To: <1508437836-31649-1-git-send-email-dmilburn@redhat.com>
This patch prevents the error handler from changing link->sata_spd unless the
link is online.
(debug output)
[ 79.658098] ATA_EH_RESET: sstatus 0x0 link->sata_spd 0x0
[ 79.658104] SATA_PRINT_LINK_STATUS: sstatus 0x0 scontrol 0x300
[ 79.658109] ata4: SATA link down (SStatus 0 SControl 300)
[ 79.658113] ATA_PHYS_LINK_OFFLINE: sata_scr_read sstatus 0x0
[ 79.658117] ATA_PHYS_LINK_OFFLINE: sata_scr_read sstatus 0x0
[ 79.658128] SATA_DOWN_SPD_LIMIT: sstatus 0x0
[ 79.658130] SATA_DOWN_SPD_LIMIT: spd 0x0 mask 0xffffffff
[ 79.658132] SATA_DOWN_SPD_LIMIT: bit 31 mask 0x7fffffff
[ 79.658134] SATA_DOWN_SPD_LIMIT: spd 0x0 mask 0x1
[ 79.658137] ata4: limiting SATA link speed to 1.5 Gbps
Signed-off-by: David Milburn <dmilburn@redhat.com>
---
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-eh.c | 6 ++++--
drivers/ata/libata.h | 1 +
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 38c40cf..57c7e56 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -177,7 +177,7 @@ struct ata_force_ent {
MODULE_VERSION(DRV_VERSION);
-static bool ata_sstatus_online(u32 sstatus)
+bool ata_sstatus_online(u32 sstatus)
{
return (sstatus & 0xf) == 0x3;
}
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index e4effef..d469951 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2911,9 +2911,11 @@ int ata_eh_reset(struct ata_link *link, int classify,
}
/* record current link speed */
- if (sata_scr_read(link, SCR_STATUS, &sstatus) == 0)
+ if (!sata_scr_read(link, SCR_STATUS, &sstatus) &&
+ ata_sstatus_online(sstatus))
link->sata_spd = (sstatus >> 4) & 0xf;
- if (slave && sata_scr_read(slave, SCR_STATUS, &sstatus) == 0)
+ if (slave && !sata_scr_read(slave, SCR_STATUS, &sstatus) &&
+ ata_sstatus_online(sstatus))
slave->sata_spd = (sstatus >> 4) & 0xf;
/* thaw the port */
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 839d487..7787fcc 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -52,6 +52,7 @@ enum {
extern int libata_noacpi;
extern int libata_allow_tpm;
extern struct device_type ata_port_type;
+extern bool ata_sstatus_online(u32 sstatus);
extern struct ata_link *ata_dev_phys_link(struct ata_device *dev);
extern void ata_force_cbl(struct ata_port *ap);
extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] libata: initialize link speed from sstatus if online.
From: David Milburn @ 2017-10-19 18:30 UTC (permalink / raw)
To: linux-ide; +Cc: tj
In-Reply-To: <1508437836-31649-1-git-send-email-dmilburn@redhat.com>
If not, a 6Gbps link may be limited to 1.5 Gbps instead of 3.0 Gbps, since
ata_dev_init initializes link->sata_spd to 0.
[ 938.387795] ata4: exception Emask 0x50 SAct 0x0 SErr 0x4090800 action 0xe frozen
[ 938.387803] ata4: irq_stat 0x00400040, connection status changed
[ 938.387807] ata4: SError: { HostInt PHYRdyChg 10B8B DevExch }
[ 938.387814] ata4: hard resetting link
[ 939.108967] ata4: SATA link down (SStatus 0 SControl 300)
[ 944.100701] ata4: hard resetting link
[ 944.404187] ata4: SATA link down (SStatus 0 SControl 300)
[ 944.404198] ata4: limiting SATA link speed to 1.5 Gbps
[ 949.394013] ata4: hard resetting link
[ 949.697465] ata4: SATA link down (SStatus 0 SControl 310)
[ 949.697476] ata4.00: disabled
[ 949.697490] ata4: EH complete
[ 949.697503] ata4.00: detaching (SCSI 3:0:0:0)
Signed-off-by: David Milburn <dmilburn@redhat.com>
---
drivers/ata/libata-core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ee4c1ec..38c40cf 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3002,16 +3002,16 @@ int ata_bus_probe(struct ata_port *ap)
*/
static void sata_print_link_status(struct ata_link *link)
{
- u32 sstatus, scontrol, tmp;
+ u32 sstatus, scontrol;
if (sata_scr_read(link, SCR_STATUS, &sstatus))
return;
sata_scr_read(link, SCR_CONTROL, &scontrol);
if (ata_phys_link_online(link)) {
- tmp = (sstatus >> 4) & 0xf;
+ link->sata_spd = (sstatus >> 4) & 0xf;
ata_link_info(link, "SATA link up %s (SStatus %X SControl %X)\n",
- sata_spd_string(tmp), sstatus, scontrol);
+ sata_spd_string(link->sata_spd), sstatus, scontrol);
} else {
ata_link_info(link, "SATA link down (SStatus %X SControl %X)\n",
sstatus, scontrol);
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2] libata: hotplug link speed
From: David Milburn @ 2017-10-19 18:30 UTC (permalink / raw)
To: linux-ide; +Cc: tj
During hotplug, it is possible for 6Gbps link speed to
be limited all the way down to 1.5 Gbps which may lead
to a slower link speed when drive is re-connected.
This behavior has been seen on a Intel Lewisburg SATA
controller (8086:a1d2) with HGST HUH728080ALE600 drive
where SATA link speed was limited to 1.5 Gbps and
when re-connected the link came up 3.0 Gbps. ata_dev_init
initializes link->spd to 0, but, the 0001 patch will
reset it once the link comes online. The 0002 patch will
allow the error handler to reset link->spd only if the
link is online; otherwise, it will always get set to 0.
This patch set was retested on above configuration and
showed the hotplugged link to come back online at max
speed (6Gbps). I did not see the downgrade when testing
on Intel C600/X79, but retested patched linux-4.14-rc5
kernel and didn't see any side effects from these
changes. Also, successfully retested hotplug on port
multiplier 3Gbps link.
^ permalink raw reply
* Re: [PATCH V8 4/5] libsas: Align SMP req/resp to dma_get_cache_alignment()
From: Christoph Hellwig @ 2017-10-19 15:12 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Huacai Chen, Christoph Hellwig, Robin Murphy, Andrew Morton,
Fuxin Zhang, linux-kernel, Ralf Baechle, James Hogan, linux-mips,
James E . J . Bottomley, Martin K . Petersen, linux-scsi,
Tejun Heo, linux-ide, stable
In-Reply-To: <286bf838-4d2f-a25f-baf9-ef3ac9b37d93@samsung.com>
On Tue, Oct 17, 2017 at 01:55:43PM +0200, Marek Szyprowski wrote:
> If I remember correctly, kernel guarantees that each kmalloced buffer is
> always at least aligned to the CPU cache line, so CPU cache can be
> invalidated on the allocated buffer without corrupting anything else.
Yes, from slab.h:
/*
* Some archs want to perform DMA into kmalloc caches and need a guaranteed
* alignment larger than the alignment of a 64-bit integer.
* Setting ARCH_KMALLOC_MINALIGN in arch headers allows that.
*/
#if defined(ARCH_DMA_MINALIGN) && ARCH_DMA_MINALIGN > 8
#define ARCH_KMALLOC_MINALIGN ARCH_DMA_MINALIGN
#define KMALLOC_MIN_SIZE ARCH_DMA_MINALIGN
#define KMALLOC_SHIFT_LOW ilog2(ARCH_DMA_MINALIGN)
#else
#define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
#endif
Mips sets this for a few subarchitectures, but it seems like you need
to set it for yours as well.
^ permalink raw reply
* Re: [PATCH V8 3/5] scsi: Align block queue to dma_get_cache_alignment()
From: Christoph Hellwig @ 2017-10-19 15:10 UTC (permalink / raw)
To: Huacai Chen
Cc: Christoph Hellwig, Marek Szyprowski, Robin Murphy, Andrew Morton,
Fuxin Zhang, linux-kernel, Ralf Baechle, James Hogan, linux-mips,
James E . J . Bottomley, Martin K . Petersen, linux-scsi,
Tejun Heo, linux-ide, stable
In-Reply-To: <1508227542-13165-3-git-send-email-chenhc@lemote.com>
On Tue, Oct 17, 2017 at 04:05:40PM +0800, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so scsi's block queue should be aligned to
> ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer and a kernel structure
> share a same cache line, and if the kernel structure has dirty data,
> cache_invalidate (no writeback) will cause data corruption.
Looks fine to, and I like cleaning up the arcane 0x03 as wel.
^ permalink raw reply
* Re: [PATCH V8 1/5] dma-mapping: Rework dma_get_cache_alignment()
From: Christoph Hellwig @ 2017-10-19 15:09 UTC (permalink / raw)
To: Mark Greer
Cc: Huacai Chen, Christoph Hellwig, Marek Szyprowski, Robin Murphy,
Andrew Morton, Fuxin Zhang, linux-kernel, Ralf Baechle,
James Hogan, linux-mips, James E . J . Bottomley,
Martin K . Petersen, linux-scsi, Tejun Heo, linux-ide, stable,
Michael S . Tsirkin, Pawel Osciak, Kyungmin Park, Michael Chan,
Benjamin Herrenschmidt
In-Reply-To: <20171018172336.GA29358@animalcreek.com>
On Wed, Oct 18, 2017 at 10:23:36AM -0700, Mark Greer wrote:
> > #define MPSC_RXR_ENTRIES 32
> > -#define MPSC_RXRE_SIZE dma_get_cache_alignment()
> > +#define MPSC_RXRE_SIZE dma_get_cache_alignment(dma_dev)
>
> I would much prefer that you add a parameter to the macro to avoid forcing
> a non-flexible and non-obvious variable definition wherever it is used.
> What I mean is something like:
>
> #define MPSC_RXRE_SIZE(d) dma_get_cache_alignment(d)
>
> Similarly for all of the other macros and where they're used.
Agreed. Except for that the patch looks fine to me, though.
^ permalink raw reply
* Re: [PATCH V8 5/5] libata: Align DMA buffer to dma_get_cache_alignment()
From: Matt Redfearn @ 2017-10-19 7:52 UTC (permalink / raw)
To: Tejun Heo, Huacai Chen
Cc: Christoph Hellwig, Marek Szyprowski, Robin Murphy, Andrew Morton,
Fuxin Zhang, linux-kernel, Ralf Baechle, James Hogan, linux-mips,
James E . J . Bottomley, Martin K . Petersen, linux-scsi,
linux-ide, stable
In-Reply-To: <20171018130353.GA1302522@devbig577.frc2.facebook.com>
On 18/10/17 14:03, Tejun Heo wrote:
> On Tue, Oct 17, 2017 at 04:05:42PM +0800, Huacai Chen wrote:
>> In non-coherent DMA mode, kernel uses cache flushing operations to
>> maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer
>> should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer
>> and a kernel structure share a same cache line, and if the kernel
>> structure has dirty data, cache_invalidate (no writeback) will cause
>> data corruption.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> ---
>> drivers/ata/libata-core.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index ee4c1ec..e134955 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev)
>> unsigned int ata_do_dev_read_id(struct ata_device *dev,
>> struct ata_taskfile *tf, u16 *id)
>> {
>> - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE,
>> - id, sizeof(id[0]) * ATA_ID_WORDS, 0);
>> + u16 *devid;
>> + int res, size = sizeof(u16) * ATA_ID_WORDS;
>> +
>> + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev)))
>> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0);
>> + else {
>> + devid = kmalloc(size, GFP_KERNEL);
>> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0);
>> + memcpy(id, devid, size);
>> + kfree(devid);
>> + }
>> +
>> + return res;
> Hmm... I think it'd be a lot better to ensure that the buffers are
> aligned properly to begin with. There are only two buffers which are
> used for id reading - ata_port->sector_buf and ata_device->id. Both
> are embedded arrays but making them separately allocated aligned
> buffers shouldn't be difficult.
>
> Thanks.
FWIW, I agree that the buffers used for DMA should be split out from the
structure. We ran into this problem on MIPS last year,
4ee34ea3a12396f35b26d90a094c75db95080baa ("libata: Align ata_device's id
on a cacheline") partially fixed it, but likely should have also
cacheline aligned the following devslp_timing in the struct such that we
guarantee that members of the struct not used for DMA do not share the
same cacheline as the DMA buffer. Not having this means that
architectures, such as MIPS, which in some cases have to perform manual
invalidation of DMA buffer can clobber valid adjacent data if it is in
the same cacheline.
Thanks,
Matt
^ permalink raw reply
* Re: [PATCH V8 5/5] libata: Align DMA buffer to dma_get_cache_alignment()
From: Alan Cox @ 2017-10-18 19:54 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Huacai Chen, Christoph Hellwig, Marek Szyprowski, Robin Murphy,
Andrew Morton, Fuxin Zhang, linux-kernel, Ralf Baechle,
James Hogan, linux-mips, James E . J . Bottomley,
Martin K . Petersen, linux-scsi, Tejun Heo, linux-ide, stable
In-Reply-To: <94f55c1e-7553-0fc8-124e-ac6df5ac10ce@cogentembedded.com>
> This function is called only for the PIO mode commands, so I doubt this is
> necessary...
That is true but there are platforms out there that issue disk level PIO
commands via DMA (or can do so). Indeed the Cyrix MediaGX could do that
in the 1990s but I never add support 8)
So I think it makes sense to allocate the buffers DMA aligned, but it
doesn't seem to explain the situation in this case and I think it would
be helpful to know what platform and ATA driver is tripping this and wny
they are the only people in the universe to have the problem.
Alan
^ permalink raw reply
* Re: [PATCH V8 1/5] dma-mapping: Rework dma_get_cache_alignment()
From: Mark Greer @ 2017-10-18 17:23 UTC (permalink / raw)
To: Huacai Chen
Cc: Christoph Hellwig, Marek Szyprowski, Robin Murphy, Andrew Morton,
Fuxin Zhang, linux-kernel, Ralf Baechle, James Hogan, linux-mips,
James E . J . Bottomley, Martin K . Petersen, linux-scsi,
Tejun Heo, linux-ide, stable, Michael S . Tsirkin, Pawel Osciak,
Kyungmin Park, Michael Chan, Benjamin Herrenschmidt, Ivan
In-Reply-To: <1508227542-13165-1-git-send-email-chenhc@lemote.com>
On Tue, Oct 17, 2017 at 04:05:38PM +0800, Huacai Chen wrote:
> Make dma_get_cache_alignment() to accept a 'dev' argument. As a result,
> it can return different alignments due to different devices' I/O cache
> coherency.
>
> Currently, ARM/ARM64 and MIPS support coherent & noncoherent devices
> co-exist. This may be extended in the future, so add a new function
> pointer (i.e, get_cache_alignment) in 'struct dma_map_ops' as a generic
> solution.
Hi Huacai.
> diff --git a/drivers/tty/serial/mpsc.c b/drivers/tty/serial/mpsc.c
> index 67ffecc..c708457 100644
> --- a/drivers/tty/serial/mpsc.c
> +++ b/drivers/tty/serial/mpsc.c
> @@ -81,19 +81,19 @@
> * Number of Tx & Rx descriptors must be powers of 2.
> */
> #define MPSC_RXR_ENTRIES 32
> -#define MPSC_RXRE_SIZE dma_get_cache_alignment()
> +#define MPSC_RXRE_SIZE dma_get_cache_alignment(dma_dev)
I would much prefer that you add a parameter to the macro to avoid forcing
a non-flexible and non-obvious variable definition wherever it is used.
What I mean is something like:
#define MPSC_RXRE_SIZE(d) dma_get_cache_alignment(d)
Similarly for all of the other macros and where they're used.
Thanks,
Mark
--
^ permalink raw reply
* Re: [PATCH V8 5/5] libata: Align DMA buffer to dma_get_cache_alignment()
From: Tejun Heo @ 2017-10-18 13:03 UTC (permalink / raw)
To: Huacai Chen
Cc: Christoph Hellwig, Marek Szyprowski, Robin Murphy, Andrew Morton,
Fuxin Zhang, linux-kernel, Ralf Baechle, James Hogan, linux-mips,
James E . J . Bottomley, Martin K . Petersen, linux-scsi,
linux-ide, stable
In-Reply-To: <1508227542-13165-5-git-send-email-chenhc@lemote.com>
On Tue, Oct 17, 2017 at 04:05:42PM +0800, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer
> should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer
> and a kernel structure share a same cache line, and if the kernel
> structure has dirty data, cache_invalidate (no writeback) will cause
> data corruption.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
> drivers/ata/libata-core.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ee4c1ec..e134955 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev)
> unsigned int ata_do_dev_read_id(struct ata_device *dev,
> struct ata_taskfile *tf, u16 *id)
> {
> - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE,
> - id, sizeof(id[0]) * ATA_ID_WORDS, 0);
> + u16 *devid;
> + int res, size = sizeof(u16) * ATA_ID_WORDS;
> +
> + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev)))
> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0);
> + else {
> + devid = kmalloc(size, GFP_KERNEL);
> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0);
> + memcpy(id, devid, size);
> + kfree(devid);
> + }
> +
> + return res;
Hmm... I think it'd be a lot better to ensure that the buffers are
aligned properly to begin with. There are only two buffers which are
used for id reading - ata_port->sector_buf and ata_device->id. Both
are embedded arrays but making them separately allocated aligned
buffers shouldn't be difficult.
Thanks.
--
tejun
^ permalink raw reply
* Re: BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
From: Yu Chen @ 2017-10-18 8:57 UTC (permalink / raw)
To: linux-ide
In-Reply-To: <CADjb_WS86DytQ1FFMG-ucv2Pr282hacGzqpeW0_aHjON=_j5+g@mail.gmail.com>
On Wed, Oct 18, 2017 at 4:56 PM, Yu Chen <yu.chen.surf@gmail.com> wrote:
> I saw the following backtrace when running some benchmarks, it might
> not be critical, but it is even better we can get rid of this warning : )
>
>
> [ 318.808335] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:238
> [ 318.818779] in_atomic(): 1, irqs_disabled(): 1, pid: 1144, name: scsi_eh_7
> [ 318.826248] CPU: 5 PID: 1144 Comm: scsi_eh_7 Tainted: G O
> 4.14.0-rc4-00128-g30e9e23 #2
> [ 318.835882] Call Trace:
> [ 318.838973] dump_stack+0x63/0x86
> [ 318.842882] ___might_sleep+0xf1/0x110
> [ 318.847214] __might_sleep+0x4a/0x80
> [ 318.851368] mutex_lock+0x20/0x50
> [ 318.855247] kernfs_find_and_get_ns+0x23/0x60
> [ 318.860158] sysfs_notify+0x77/0x90
> [ 318.864237] scsi_device_set_state+0x63/0x150
> [ 318.869180] ata_scsi_offline_dev+0x1c/0x30 [libata]
> [ 318.874696] ata_eh_detach_dev+0x3b/0xb0 [libata]
> [ 318.879929] ata_eh_schedule_probe+0x59/0x1c0 [libata]
> [ 318.885590] ata_eh_recover+0x108/0x12d0 [libata]
> [ 318.890807] ? ahci_pmp_attach+0x70/0x70 [libahci]
> [ 318.896105] ? ahci_do_hardreset+0x110/0x110 [libahci]
> [ 318.901744] ? ahci_do_softreset+0x210/0x210 [libahci]
> [ 318.907371] ? ata_phys_link_offline+0x30/0x30 [libata]
> [ 318.913123] ? ata_eh_report+0x34f/0x850 [libata]
> [ 318.918345] sata_pmp_eh_recover+0xb7/0xa00 [libata]
> [ 318.923818] ? ata_eh_report+0x6e6/0x850 [libata]
> [ 318.929028] sata_pmp_error_handler+0x22/0x30 [libata]
> [ 318.934631] ahci_error_handler+0x1d/0x70 [libahci]
> [ 318.939968] ata_scsi_port_error_handler+0x430/0x720 [libata]
> [ 318.946167] ? ata_scsi_cmd_error_handler+0xe9/0x140 [libata]
> [ 318.952368] ata_scsi_error+0x86/0xb0 [libata]
> [ 318.957260] ? scsi_error_handler+0x38/0x5d0
> [ 318.961965] scsi_error_handler+0xe7/0x5d0
> [ 318.966499] kthread+0x114/0x150
> [ 318.970155] ? scsi_eh_get_sense+0x280/0x280
> [ 318.974848] ? kthread_create_on_node+0x40/0x40
> [ 318.979798] ret_from_fork+0x25/0x30
> [ 318.983813] ata8: hard resetting link
>
> Thanks,
> Yu
Seen on 4.14-rc4
^ permalink raw reply
* BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
From: Yu Chen @ 2017-10-18 8:56 UTC (permalink / raw)
To: linux-ide
I saw the following backtrace when running some benchmarks, it might
not be critical, but it is even better we can get rid of this warning : )
[ 318.808335] BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:238
[ 318.818779] in_atomic(): 1, irqs_disabled(): 1, pid: 1144, name: scsi_eh_7
[ 318.826248] CPU: 5 PID: 1144 Comm: scsi_eh_7 Tainted: G O
4.14.0-rc4-00128-g30e9e23 #2
[ 318.835882] Call Trace:
[ 318.838973] dump_stack+0x63/0x86
[ 318.842882] ___might_sleep+0xf1/0x110
[ 318.847214] __might_sleep+0x4a/0x80
[ 318.851368] mutex_lock+0x20/0x50
[ 318.855247] kernfs_find_and_get_ns+0x23/0x60
[ 318.860158] sysfs_notify+0x77/0x90
[ 318.864237] scsi_device_set_state+0x63/0x150
[ 318.869180] ata_scsi_offline_dev+0x1c/0x30 [libata]
[ 318.874696] ata_eh_detach_dev+0x3b/0xb0 [libata]
[ 318.879929] ata_eh_schedule_probe+0x59/0x1c0 [libata]
[ 318.885590] ata_eh_recover+0x108/0x12d0 [libata]
[ 318.890807] ? ahci_pmp_attach+0x70/0x70 [libahci]
[ 318.896105] ? ahci_do_hardreset+0x110/0x110 [libahci]
[ 318.901744] ? ahci_do_softreset+0x210/0x210 [libahci]
[ 318.907371] ? ata_phys_link_offline+0x30/0x30 [libata]
[ 318.913123] ? ata_eh_report+0x34f/0x850 [libata]
[ 318.918345] sata_pmp_eh_recover+0xb7/0xa00 [libata]
[ 318.923818] ? ata_eh_report+0x6e6/0x850 [libata]
[ 318.929028] sata_pmp_error_handler+0x22/0x30 [libata]
[ 318.934631] ahci_error_handler+0x1d/0x70 [libahci]
[ 318.939968] ata_scsi_port_error_handler+0x430/0x720 [libata]
[ 318.946167] ? ata_scsi_cmd_error_handler+0xe9/0x140 [libata]
[ 318.952368] ata_scsi_error+0x86/0xb0 [libata]
[ 318.957260] ? scsi_error_handler+0x38/0x5d0
[ 318.961965] scsi_error_handler+0xe7/0x5d0
[ 318.966499] kthread+0x114/0x150
[ 318.970155] ? scsi_eh_get_sense+0x280/0x280
[ 318.974848] ? kthread_create_on_node+0x40/0x40
[ 318.979798] ret_from_fork+0x25/0x30
[ 318.983813] ata8: hard resetting link
Thanks,
Yu
^ permalink raw reply
* Re: [PATCH V8 4/5] libsas: Align SMP req/resp todma_get_cache_alignment()
From: 陈华才 @ 2017-10-18 1:12 UTC (permalink / raw)
To: Marek Szyprowski, Christoph Hellwig
Cc: Robin Murphy, Andrew Morton, Fuxin Zhang, linux-kernel,
Ralf Baechle, JamesHogan, linux-mips, James E . J .Bottomley,
Martin K . Petersen, linux-scsi, Tejun Heo, linux-ide, stable
In-Reply-To: <286bf838-4d2f-a25f-baf9-ef3ac9b37d93@samsung.com>
Hi, Marek,
Yes, I know in include/linux/slab.h, there is
#define KMALLOC_MIN_SIZE ARCH_DMA_MINALIGN
But I observed that the req/resp isn't aligned to ARCH_DMA_MINALIGN and I don't know why.
Problems I experienced is: In non-coherent mode, mvsas with an expander cannot detect any disks.
Huacai
------------------ Original ------------------
From: "Marek Szyprowski"<m.szyprowski@samsung.com>;
Date: Tue, Oct 17, 2017 07:55 PM
To: "Huacai Chen"<chenhc@lemote.com>; "Christoph Hellwig"<hch@lst.de>;
Cc: "Robin Murphy"<robin.murphy@arm.com>; "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-kernel"<linux-kernel@vger.kernel.org>; "Ralf Baechle"<ralf@linux-mips.org>; "JamesHogan"<james.hogan@imgtec.com>; "linux-mips"<linux-mips@linux-mips.org>; "James E . J .Bottomley"<jejb@linux.vnet.ibm.com>; "Martin K . Petersen"<martin.petersen@oracle.com>; "linux-scsi"<linux-scsi@vger.kernel.org>; "Tejun Heo"<tj@kernel.org>; "linux-ide"<linux-ide@vger.kernel.org>; "stable"<stable@vger.kernel.org>;
Subject: Re: [PATCH V8 4/5] libsas: Align SMP req/resp todma_get_cache_alignment()
Hi Huacai,
On 2017-10-17 10:05, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so libsas's SMP request/response should be
> aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer and a kernel
> structure share a same cache line, and if the kernel structure has
> dirty data, cache_invalidate (no writeback) will cause data corruption.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
> drivers/scsi/libsas/sas_expander.c | 93 +++++++++++++++++++++++---------------
> 1 file changed, 57 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 6b4fd23..124a44b 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -164,17 +164,17 @@ static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
>
> /* ---------- Allocations ---------- */
>
> -static inline void *alloc_smp_req(int size)
> +static inline void *alloc_smp_req(int size, int align)
> {
> - u8 *p = kzalloc(size, GFP_KERNEL);
> + u8 *p = kzalloc(ALIGN(size, align), GFP_KERNEL);
If I remember correctly, kernel guarantees that each kmalloced buffer is
always at least aligned to the CPU cache line, so CPU cache can be
invalidated on the allocated buffer without corrupting anything else.
Taking this into account, I wonder if the above change make sense.
Have you experienced any problems without this change?
> if (p)
> p[0] = SMP_REQUEST;
> return p;
> }
>
> -static inline void *alloc_smp_resp(int size)
> +static inline void *alloc_smp_resp(int size, int align)
> {
> - return kzalloc(size, GFP_KERNEL);
> + return kzalloc(ALIGN(size, align), GFP_KERNEL);
Save a above.
> }
>
> static char sas_route_char(struct domain_device *dev, struct ex_phy *phy)
> @@ -403,15 +403,17 @@ static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req,
> int sas_ex_phy_discover(struct domain_device *dev, int single)
> {
> struct expander_device *ex = &dev->ex_dev;
> - int res = 0;
> + int res = 0, align;
> u8 *disc_req;
> u8 *disc_resp;
>
> - disc_req = alloc_smp_req(DISCOVER_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_req = alloc_smp_req(DISCOVER_REQ_SIZE, align);
> if (!disc_req)
> return -ENOMEM;
>
> - disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> + disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE, align);
> if (!disc_resp) {
> kfree(disc_req);
> return -ENOMEM;
> @@ -480,14 +482,15 @@ static int sas_ex_general(struct domain_device *dev)
> {
> u8 *rg_req;
> struct smp_resp *rg_resp;
> - int res;
> - int i;
> + int i, res, align;
>
> - rg_req = alloc_smp_req(RG_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + rg_req = alloc_smp_req(RG_REQ_SIZE, align);
> if (!rg_req)
> return -ENOMEM;
>
> - rg_resp = alloc_smp_resp(RG_RESP_SIZE);
> + rg_resp = alloc_smp_resp(RG_RESP_SIZE, align);
> if (!rg_resp) {
> kfree(rg_req);
> return -ENOMEM;
> @@ -552,13 +555,15 @@ static int sas_ex_manuf_info(struct domain_device *dev)
> {
> u8 *mi_req;
> u8 *mi_resp;
> - int res;
> + int res, align;
>
> - mi_req = alloc_smp_req(MI_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + mi_req = alloc_smp_req(MI_REQ_SIZE, align);
> if (!mi_req)
> return -ENOMEM;
>
> - mi_resp = alloc_smp_resp(MI_RESP_SIZE);
> + mi_resp = alloc_smp_resp(MI_RESP_SIZE, align);
> if (!mi_resp) {
> kfree(mi_req);
> return -ENOMEM;
> @@ -593,13 +598,15 @@ int sas_smp_phy_control(struct domain_device *dev, int phy_id,
> {
> u8 *pc_req;
> u8 *pc_resp;
> - int res;
> + int res, align;
> +
> + align = dma_get_cache_alignment(&dev->phy->dev);
>
> - pc_req = alloc_smp_req(PC_REQ_SIZE);
> + pc_req = alloc_smp_req(PC_REQ_SIZE, align);
> if (!pc_req)
> return -ENOMEM;
>
> - pc_resp = alloc_smp_resp(PC_RESP_SIZE);
> + pc_resp = alloc_smp_resp(PC_RESP_SIZE, align);
> if (!pc_resp) {
> kfree(pc_req);
> return -ENOMEM;
> @@ -664,17 +671,19 @@ static int sas_dev_present_in_domain(struct asd_sas_port *port,
> #define RPEL_RESP_SIZE 32
> int sas_smp_get_phy_events(struct sas_phy *phy)
> {
> - int res;
> + int res, align;
> u8 *req;
> u8 *resp;
> struct sas_rphy *rphy = dev_to_rphy(phy->dev.parent);
> struct domain_device *dev = sas_find_dev_by_rphy(rphy);
>
> - req = alloc_smp_req(RPEL_REQ_SIZE);
> + align = dma_get_cache_alignment(&phy->dev);
> +
> + req = alloc_smp_req(RPEL_REQ_SIZE, align);
> if (!req)
> return -ENOMEM;
>
> - resp = alloc_smp_resp(RPEL_RESP_SIZE);
> + resp = alloc_smp_resp(RPEL_RESP_SIZE, align);
> if (!resp) {
> kfree(req);
> return -ENOMEM;
> @@ -709,7 +718,8 @@ int sas_get_report_phy_sata(struct domain_device *dev, int phy_id,
> struct smp_resp *rps_resp)
> {
> int res;
> - u8 *rps_req = alloc_smp_req(RPS_REQ_SIZE);
> + int align = dma_get_cache_alignment(&dev->phy->dev);
> + u8 *rps_req = alloc_smp_req(RPS_REQ_SIZE, align);
> u8 *resp = (u8 *)rps_resp;
>
> if (!rps_req)
> @@ -1398,7 +1408,7 @@ static int sas_check_parent_topology(struct domain_device *child)
> static int sas_configure_present(struct domain_device *dev, int phy_id,
> u8 *sas_addr, int *index, int *present)
> {
> - int i, res = 0;
> + int i, res = 0, align;
> struct expander_device *ex = &dev->ex_dev;
> struct ex_phy *phy = &ex->ex_phy[phy_id];
> u8 *rri_req;
> @@ -1406,12 +1416,13 @@ static int sas_configure_present(struct domain_device *dev, int phy_id,
>
> *present = 0;
> *index = 0;
> + align = dma_get_cache_alignment(&dev->phy->dev);
>
> - rri_req = alloc_smp_req(RRI_REQ_SIZE);
> + rri_req = alloc_smp_req(RRI_REQ_SIZE, align);
> if (!rri_req)
> return -ENOMEM;
>
> - rri_resp = alloc_smp_resp(RRI_RESP_SIZE);
> + rri_resp = alloc_smp_resp(RRI_RESP_SIZE, align);
> if (!rri_resp) {
> kfree(rri_req);
> return -ENOMEM;
> @@ -1472,15 +1483,17 @@ static int sas_configure_present(struct domain_device *dev, int phy_id,
> static int sas_configure_set(struct domain_device *dev, int phy_id,
> u8 *sas_addr, int index, int include)
> {
> - int res;
> + int res, align;
> u8 *cri_req;
> u8 *cri_resp;
>
> - cri_req = alloc_smp_req(CRI_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + cri_req = alloc_smp_req(CRI_REQ_SIZE, align);
> if (!cri_req)
> return -ENOMEM;
>
> - cri_resp = alloc_smp_resp(CRI_RESP_SIZE);
> + cri_resp = alloc_smp_resp(CRI_RESP_SIZE, align);
> if (!cri_resp) {
> kfree(cri_req);
> return -ENOMEM;
> @@ -1689,10 +1702,12 @@ int sas_discover_root_expander(struct domain_device *dev)
> static int sas_get_phy_discover(struct domain_device *dev,
> int phy_id, struct smp_resp *disc_resp)
> {
> - int res;
> + int res, align;
> u8 *disc_req;
>
> - disc_req = alloc_smp_req(DISCOVER_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_req = alloc_smp_req(DISCOVER_REQ_SIZE, align);
> if (!disc_req)
> return -ENOMEM;
>
> @@ -1715,10 +1730,12 @@ static int sas_get_phy_discover(struct domain_device *dev,
> static int sas_get_phy_change_count(struct domain_device *dev,
> int phy_id, int *pcc)
> {
> - int res;
> + int res, align;
> struct smp_resp *disc_resp;
>
> - disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE, align);
> if (!disc_resp)
> return -ENOMEM;
>
> @@ -1733,11 +1750,13 @@ static int sas_get_phy_change_count(struct domain_device *dev,
> static int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
> u8 *sas_addr, enum sas_device_type *type)
> {
> - int res;
> + int res, align;
> struct smp_resp *disc_resp;
> struct discover_resp *dr;
>
> - disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE, align);
> if (!disc_resp)
> return -ENOMEM;
> dr = &disc_resp->disc;
> @@ -1787,15 +1806,17 @@ static int sas_find_bcast_phy(struct domain_device *dev, int *phy_id,
>
> static int sas_get_ex_change_count(struct domain_device *dev, int *ecc)
> {
> - int res;
> + int res, align;
> u8 *rg_req;
> struct smp_resp *rg_resp;
>
> - rg_req = alloc_smp_req(RG_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + rg_req = alloc_smp_req(RG_REQ_SIZE, align);
> if (!rg_req)
> return -ENOMEM;
>
> - rg_resp = alloc_smp_resp(RG_RESP_SIZE);
> + rg_resp = alloc_smp_resp(RG_RESP_SIZE, align);
> if (!rg_resp) {
> kfree(rg_req);
> return -ENOMEM;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH V8 5/5] libata: Align DMA buffer todma_get_cache_alignment()
From: 陈华才 @ 2017-10-18 1:06 UTC (permalink / raw)
To: Sergei Shtylyov, Christoph Hellwig
Cc: Marek Szyprowski, Robin Murphy, Andrew Morton, Fuxin Zhang,
linux-kernel, Ralf Baechle, James Hogan, linux-mips,
James E . J . Bottomley, Martin K . Petersen, linux-scsi,
Tejun Heo, linux-ide
In-Reply-To: <94f55c1e-7553-0fc8-124e-ac6df5ac10ce@cogentembedded.com>
Hi, Sergei,
Without this patch, in non-coherent mode, ata_do_set_mode() return with "no PIO support", and disk detection fails.
Huacai
------------------ Original ------------------
From: "Sergei Shtylyov"<sergei.shtylyov@cogentembedded.com>;
Date: Tue, Oct 17, 2017 05:43 PM
To: "Huacai Chen"<chenhc@lemote.com>; "Christoph Hellwig"<hch@lst.de>;
Cc: "Marek Szyprowski"<m.szyprowski@samsung.com>; "Robin Murphy"<robin.murphy@arm.com>; "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-kernel"<linux-kernel@vger.kernel.org>; "Ralf Baechle"<ralf@linux-mips.org>; "James Hogan"<james.hogan@imgtec.com>; "linux-mips"<linux-mips@linux-mips.org>; "James E . J . Bottomley"<jejb@linux.vnet.ibm.com>; "Martin K . Petersen"<martin.petersen@oracle.com>; "linux-scsi"<linux-scsi@vger.kernel.org>; "Tejun Heo"<tj@kernel.org>; "linux-ide"<linux-ide@vger.kernel.org>; "stable"<stable@vger.kernel.org>;
Subject: Re: [PATCH V8 5/5] libata: Align DMA buffer todma_get_cache_alignment()
On 10/17/2017 11:05 AM, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer
> should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer
> and a kernel structure share a same cache line, and if the kernel
> structure has dirty data, cache_invalidate (no writeback) will cause
> data corruption.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
> drivers/ata/libata-core.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ee4c1ec..e134955 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev)
> unsigned int ata_do_dev_read_id(struct ata_device *dev,
> struct ata_taskfile *tf, u16 *id)
> {
> - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE,
> - id, sizeof(id[0]) * ATA_ID_WORDS, 0);
> + u16 *devid;
> + int res, size = sizeof(u16) * ATA_ID_WORDS;
> +
> + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev)))
> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0);
> + else {
> + devid = kmalloc(size, GFP_KERNEL);
> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0);
> + memcpy(id, devid, size);
> + kfree(devid);
> + }
> +
> + return res;
This function is called only for the PIO mode commands, so I doubt this is
necessary...
MBR, Sergei
^ permalink raw reply
* (unknown),
From: dengx @ 2017-10-17 12:14 UTC (permalink / raw)
To: linux-ide
[-- Attachment #1: 1223614.zip --]
[-- Type: application/zip, Size: 35489 bytes --]
^ permalink raw reply
* Re: [PATCH V8 4/5] libsas: Align SMP req/resp to dma_get_cache_alignment()
From: Marek Szyprowski @ 2017-10-17 11:55 UTC (permalink / raw)
To: Huacai Chen, Christoph Hellwig
Cc: Robin Murphy, Andrew Morton, Fuxin Zhang, linux-kernel,
Ralf Baechle, James Hogan, linux-mips, James E . J . Bottomley,
Martin K . Petersen, linux-scsi, Tejun Heo, linux-ide, stable
In-Reply-To: <1508227542-13165-4-git-send-email-chenhc@lemote.com>
Hi Huacai,
On 2017-10-17 10:05, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so libsas's SMP request/response should be
> aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer and a kernel
> structure share a same cache line, and if the kernel structure has
> dirty data, cache_invalidate (no writeback) will cause data corruption.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
> drivers/scsi/libsas/sas_expander.c | 93 +++++++++++++++++++++++---------------
> 1 file changed, 57 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 6b4fd23..124a44b 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -164,17 +164,17 @@ static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
>
> /* ---------- Allocations ---------- */
>
> -static inline void *alloc_smp_req(int size)
> +static inline void *alloc_smp_req(int size, int align)
> {
> - u8 *p = kzalloc(size, GFP_KERNEL);
> + u8 *p = kzalloc(ALIGN(size, align), GFP_KERNEL);
If I remember correctly, kernel guarantees that each kmalloced buffer is
always at least aligned to the CPU cache line, so CPU cache can be
invalidated on the allocated buffer without corrupting anything else.
Taking this into account, I wonder if the above change make sense.
Have you experienced any problems without this change?
> if (p)
> p[0] = SMP_REQUEST;
> return p;
> }
>
> -static inline void *alloc_smp_resp(int size)
> +static inline void *alloc_smp_resp(int size, int align)
> {
> - return kzalloc(size, GFP_KERNEL);
> + return kzalloc(ALIGN(size, align), GFP_KERNEL);
Save a above.
> }
>
> static char sas_route_char(struct domain_device *dev, struct ex_phy *phy)
> @@ -403,15 +403,17 @@ static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req,
> int sas_ex_phy_discover(struct domain_device *dev, int single)
> {
> struct expander_device *ex = &dev->ex_dev;
> - int res = 0;
> + int res = 0, align;
> u8 *disc_req;
> u8 *disc_resp;
>
> - disc_req = alloc_smp_req(DISCOVER_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_req = alloc_smp_req(DISCOVER_REQ_SIZE, align);
> if (!disc_req)
> return -ENOMEM;
>
> - disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> + disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE, align);
> if (!disc_resp) {
> kfree(disc_req);
> return -ENOMEM;
> @@ -480,14 +482,15 @@ static int sas_ex_general(struct domain_device *dev)
> {
> u8 *rg_req;
> struct smp_resp *rg_resp;
> - int res;
> - int i;
> + int i, res, align;
>
> - rg_req = alloc_smp_req(RG_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + rg_req = alloc_smp_req(RG_REQ_SIZE, align);
> if (!rg_req)
> return -ENOMEM;
>
> - rg_resp = alloc_smp_resp(RG_RESP_SIZE);
> + rg_resp = alloc_smp_resp(RG_RESP_SIZE, align);
> if (!rg_resp) {
> kfree(rg_req);
> return -ENOMEM;
> @@ -552,13 +555,15 @@ static int sas_ex_manuf_info(struct domain_device *dev)
> {
> u8 *mi_req;
> u8 *mi_resp;
> - int res;
> + int res, align;
>
> - mi_req = alloc_smp_req(MI_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + mi_req = alloc_smp_req(MI_REQ_SIZE, align);
> if (!mi_req)
> return -ENOMEM;
>
> - mi_resp = alloc_smp_resp(MI_RESP_SIZE);
> + mi_resp = alloc_smp_resp(MI_RESP_SIZE, align);
> if (!mi_resp) {
> kfree(mi_req);
> return -ENOMEM;
> @@ -593,13 +598,15 @@ int sas_smp_phy_control(struct domain_device *dev, int phy_id,
> {
> u8 *pc_req;
> u8 *pc_resp;
> - int res;
> + int res, align;
> +
> + align = dma_get_cache_alignment(&dev->phy->dev);
>
> - pc_req = alloc_smp_req(PC_REQ_SIZE);
> + pc_req = alloc_smp_req(PC_REQ_SIZE, align);
> if (!pc_req)
> return -ENOMEM;
>
> - pc_resp = alloc_smp_resp(PC_RESP_SIZE);
> + pc_resp = alloc_smp_resp(PC_RESP_SIZE, align);
> if (!pc_resp) {
> kfree(pc_req);
> return -ENOMEM;
> @@ -664,17 +671,19 @@ static int sas_dev_present_in_domain(struct asd_sas_port *port,
> #define RPEL_RESP_SIZE 32
> int sas_smp_get_phy_events(struct sas_phy *phy)
> {
> - int res;
> + int res, align;
> u8 *req;
> u8 *resp;
> struct sas_rphy *rphy = dev_to_rphy(phy->dev.parent);
> struct domain_device *dev = sas_find_dev_by_rphy(rphy);
>
> - req = alloc_smp_req(RPEL_REQ_SIZE);
> + align = dma_get_cache_alignment(&phy->dev);
> +
> + req = alloc_smp_req(RPEL_REQ_SIZE, align);
> if (!req)
> return -ENOMEM;
>
> - resp = alloc_smp_resp(RPEL_RESP_SIZE);
> + resp = alloc_smp_resp(RPEL_RESP_SIZE, align);
> if (!resp) {
> kfree(req);
> return -ENOMEM;
> @@ -709,7 +718,8 @@ int sas_get_report_phy_sata(struct domain_device *dev, int phy_id,
> struct smp_resp *rps_resp)
> {
> int res;
> - u8 *rps_req = alloc_smp_req(RPS_REQ_SIZE);
> + int align = dma_get_cache_alignment(&dev->phy->dev);
> + u8 *rps_req = alloc_smp_req(RPS_REQ_SIZE, align);
> u8 *resp = (u8 *)rps_resp;
>
> if (!rps_req)
> @@ -1398,7 +1408,7 @@ static int sas_check_parent_topology(struct domain_device *child)
> static int sas_configure_present(struct domain_device *dev, int phy_id,
> u8 *sas_addr, int *index, int *present)
> {
> - int i, res = 0;
> + int i, res = 0, align;
> struct expander_device *ex = &dev->ex_dev;
> struct ex_phy *phy = &ex->ex_phy[phy_id];
> u8 *rri_req;
> @@ -1406,12 +1416,13 @@ static int sas_configure_present(struct domain_device *dev, int phy_id,
>
> *present = 0;
> *index = 0;
> + align = dma_get_cache_alignment(&dev->phy->dev);
>
> - rri_req = alloc_smp_req(RRI_REQ_SIZE);
> + rri_req = alloc_smp_req(RRI_REQ_SIZE, align);
> if (!rri_req)
> return -ENOMEM;
>
> - rri_resp = alloc_smp_resp(RRI_RESP_SIZE);
> + rri_resp = alloc_smp_resp(RRI_RESP_SIZE, align);
> if (!rri_resp) {
> kfree(rri_req);
> return -ENOMEM;
> @@ -1472,15 +1483,17 @@ static int sas_configure_present(struct domain_device *dev, int phy_id,
> static int sas_configure_set(struct domain_device *dev, int phy_id,
> u8 *sas_addr, int index, int include)
> {
> - int res;
> + int res, align;
> u8 *cri_req;
> u8 *cri_resp;
>
> - cri_req = alloc_smp_req(CRI_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + cri_req = alloc_smp_req(CRI_REQ_SIZE, align);
> if (!cri_req)
> return -ENOMEM;
>
> - cri_resp = alloc_smp_resp(CRI_RESP_SIZE);
> + cri_resp = alloc_smp_resp(CRI_RESP_SIZE, align);
> if (!cri_resp) {
> kfree(cri_req);
> return -ENOMEM;
> @@ -1689,10 +1702,12 @@ int sas_discover_root_expander(struct domain_device *dev)
> static int sas_get_phy_discover(struct domain_device *dev,
> int phy_id, struct smp_resp *disc_resp)
> {
> - int res;
> + int res, align;
> u8 *disc_req;
>
> - disc_req = alloc_smp_req(DISCOVER_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_req = alloc_smp_req(DISCOVER_REQ_SIZE, align);
> if (!disc_req)
> return -ENOMEM;
>
> @@ -1715,10 +1730,12 @@ static int sas_get_phy_discover(struct domain_device *dev,
> static int sas_get_phy_change_count(struct domain_device *dev,
> int phy_id, int *pcc)
> {
> - int res;
> + int res, align;
> struct smp_resp *disc_resp;
>
> - disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE, align);
> if (!disc_resp)
> return -ENOMEM;
>
> @@ -1733,11 +1750,13 @@ static int sas_get_phy_change_count(struct domain_device *dev,
> static int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
> u8 *sas_addr, enum sas_device_type *type)
> {
> - int res;
> + int res, align;
> struct smp_resp *disc_resp;
> struct discover_resp *dr;
>
> - disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE, align);
> if (!disc_resp)
> return -ENOMEM;
> dr = &disc_resp->disc;
> @@ -1787,15 +1806,17 @@ static int sas_find_bcast_phy(struct domain_device *dev, int *phy_id,
>
> static int sas_get_ex_change_count(struct domain_device *dev, int *ecc)
> {
> - int res;
> + int res, align;
> u8 *rg_req;
> struct smp_resp *rg_resp;
>
> - rg_req = alloc_smp_req(RG_REQ_SIZE);
> + align = dma_get_cache_alignment(&dev->phy->dev);
> +
> + rg_req = alloc_smp_req(RG_REQ_SIZE, align);
> if (!rg_req)
> return -ENOMEM;
>
> - rg_resp = alloc_smp_resp(RG_RESP_SIZE);
> + rg_resp = alloc_smp_resp(RG_RESP_SIZE, align);
> if (!rg_resp) {
> kfree(rg_req);
> return -ENOMEM;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox