Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753293AbcLIGk7 (ORCPT ); Fri, 9 Dec 2016 01:40:59 -0500 Received: from mail-sn1nam02on0087.outbound.protection.outlook.com ([104.47.36.87]:61520 "EHLO NAM02-SN1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752399AbcLIGk5 (ORCPT ); Fri, 9 Dec 2016 01:40:57 -0500 From: "Madhani, Himanshu" To: "Michael S. Tsirkin" , Bart Van Assche CC: "kvm@vger.kernel.org" , Neil Armstrong , David Airlie , "linux-remoteproc@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "virtualization@lists.linux-foundation.org" , "linux-s390@vger.kernel.org" , "James E.J. Bottomley" , Herbert Xu , "linux-scsi@vger.kernel.org" , Christoph Hellwig , "v9fs-developer@lists.sourceforge.net" , Asias He , "Arnd Bergmann" , "linux-kbuild@vger.kernel.org" , Jens Axboe , Michal Marek , Stefan Hajnoczi , Matt Mackall , Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" , "linux-crypto@vger.kernel.org" , "netdev@vger.kernel.org" , Linus Torvalds , "David S. Miller" Subject: Re: [PATCH] linux/types.h: enable endian checks for all sparse builds Thread-Topic: [PATCH] linux/types.h: enable endian checks for all sparse builds Thread-Index: AQHSUPr7K9TPa7K+3k2Sh5GOdwNf1aD+OzIAgABrGYA= Date: Fri, 9 Dec 2016 06:40:53 +0000 Message-ID: <6199215E-2AA4-4705-9552-5D61FE03F866@cavium.com> References: <1481164052-28036-1-git-send-email-mst@redhat.com> <20161208075152-mutt-send-email-mst@kernel.org> <20161208163658-mutt-send-email-mst@kernel.org> In-Reply-To: <20161208163658-mutt-send-email-mst@kernel.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Himanshu.Madhani@cavium.com; x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [173.186.134.106] x-ms-office365-filtering-correlation-id: 4991bbe1-2c38-4247-0efc-08d41ffe5329 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001);SRVR:SN1PR0701MB1837; x-microsoft-exchange-diagnostics: 1;SN1PR0701MB1837;7:yZzCMGQqN/Az72+6f8RFQQfUvG2OzRis/CoBzFlUyVQOqVrCtVo4UoqfUKJhO1bT2Wyu5Rg3P3BaaPbaZxq5OefjiHuMcNG5nEC3DePw0ojP+9+h+f9nLSL4knXkKgOR7XThR4rs4fKb9+tLIH4PTzm9SdvTucBm70S2/gtqm7Uhfzi9h44WKASyi4Lopd91fcEi4tJ3M1fvsvB5Xlc6ZL2EYsffFZwcm0PbU/724XNyKhqkgQAtOaVql4BQnLMbvI8MrsEsmyQJZg/X3zPGbGLcXPkdWXrIen7I6ZcqE4X5+244KQ7fP8c5jYiCq1x3+6xhHei/o5piBjJFXzYFsJyyUoIKKBbLQSTW33KiI7WVatU6Qc1/0SryERqAhkSYAuvVSJjhxCIFDDeA1MOUphSoKnIFInRreEDai0JR3vg54KtmVIB5CTCm3ujOXEhb5sKHDcyP3+/dKMXdtGwCdA== x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040375)(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001)(6041248)(20161123564025)(20161123562025)(20161123555025)(20161123560025)(6072148);SRVR:SN1PR0701MB1837;BCL:0;PCL:0;RULEID:;SRVR:SN1PR0701MB1837; x-forefront-prvs: 015114592F x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(7916002)(39840400002)(39450400003)(39410400002)(39850400002)(189002)(199003)(52544003)(377454003)(24454002)(3280700002)(3660700001)(2900100001)(36756003)(101416001)(92566002)(105586002)(99286002)(2906002)(106356001)(106116001)(38730400001)(229853002)(33656002)(77096006)(4326007)(6512006)(6506006)(1691005)(3846002)(102836003)(6116002)(68736007)(54356999)(50986999)(76176999)(6486002)(5660300001)(8666005)(7416002)(7736002)(305945005)(6436002)(122556002)(2950100002)(93886004)(8676002)(83716003)(8936002)(66066001)(189998001)(81156014)(81166006)(86362001)(97736004)(82746002)(5001770100001)(7059030)(104396002);DIR:OUT;SFP:1101;SCL:1;SRVR:SN1PR0701MB1837;H:SN1PR0701MB1837.namprd07.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 X-OriginatorOrg: cavium.com X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Dec 2016 06:40:53.6710 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 711e4ccf-2e9b-4bcf-a551-4094005b6194 X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR0701MB1837 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id uB96f5wD011933 Content-Length: 4863 Lines: 144 Hi Mike/Bart, On 12/8/16, 8:17 AM, "virtualization-bounces@lists.linux-foundation.org on behalf of Michael S. Tsirkin" wrote: >On Thu, Dec 08, 2016 at 06:38:11AM +0000, Bart Van Assche wrote: >> On 12/07/16 21:54, Michael S. Tsirkin wrote: >> > On Thu, Dec 08, 2016 at 05:21:47AM +0000, Bart Van Assche wrote: >> >> Additionally, there are notable exceptions to the rule that most drivers >> >> are endian-clean, e.g. drivers/scsi/qla2xxx. I would appreciate it if it >> >> would remain possible to check such drivers with sparse without enabling >> >> endianness checks. Have you considered to change #ifdef __CHECK_ENDIAN__ >> >> into e.g. #ifndef __DONT_CHECK_ENDIAN__? >> > >> > The right thing is probably just to fix these, isn't it? >> > Until then, why not just ignore the warnings? >> >> Neither option is realistic. With endian-checking enabled the qla2xxx >> driver triggers so many warnings that it becomes a real challenge to >> filter the non-endian warnings out manually: >> >> $ for f in "" CF=-D__CHECK_ENDIAN__; do make M=drivers/scsi/qla2xxx C=2\ >> $f | &grep -c ': warning:'; done >> 4 >> 752 > >You can always revert this patch in your tree, or whatever. It does not >look like this will get fixed otherwise. > >> If you think it would be easy to fix the endian warnings triggered by >> the qla2xxx driver, you are welcome to try to fix these. >> >> Bart. > >Yea, this hardware was designed by someone who thought mixing >LE and BE all over the place is a good idea. >But who said it should be easy? > >Maybe this change will be enough to motivate the maintainers. > >Here's a minor buglet for you as a motivator: > > if (ct_rsp->header.response != > cpu_to_be16(CT_ACCEPT_RESPONSE)) { > ql_dbg(ql_dbg_disc + ql_dbg_buffer, vha, 0x2077, > "%s failed rejected request on port_id: %02x%02x%02x Compeltion status 0x%x, response 0x%x\n", > routine, vha->d_id.b.domain, > vha->d_id.b.area, vha->d_id.b.al_pa, comp_status, ct_rsp->header.response); > > >response is BE and isn't printed correctly. > >another: > > eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size); > size += 4 + 4; > > ql_dbg(ql_dbg_disc, vha, 0x20bc, > "Max_Frame_Size = %x.\n", eiter->a.max_frame_size); > >printed too late, it's be by that time. > >Here's another suspicious line > > ctio24->u.status1.flags = (atio->u.isp24.attr << 9) | > cpu_to_le16(CTIO7_FLAGS_STATUS_MODE_1 | > CTIO7_FLAGS_TERMINATE); > >shifting attr by 9 bits gives different results on BE and LE, >mixing it with le16 looks rather strange. > >Another: > > ha->flags.dport_enabled = > (mid_init_cb->init_cb.firmware_options_1 & BIT_7) != 0; > >BIT_7 is native endian, firmware_options_1 is LE I think. > > > >Look at qla27xx_find_valid_image as well. > > if (pri_image_status.signature != QLA27XX_IMG_STATUS_SIGN) > >qla27xx_image_status seems to be data coming from flash, but is >somehow native-endian? Maybe ... > > > lun = a->u.isp24.fcp_cmnd.lun; > >I think lun here is in hardware format (le?), code treats it >as native. > > >Not to speak about interface abuse all over the place. >How about this: > >uint32_t * >qla24xx_read_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t >faddr, > uint32_t dwords) >{ > uint32_t i; > struct qla_hw_data *ha = vha->hw; > > /* Dword reads to flash. */ > for (i = 0; i < dwords; i++, faddr++) > dwptr[i] = cpu_to_le32(qla24xx_read_flash_dword(ha, > flash_data_addr(ha, faddr))); > > return dwptr; >} > >OK so we convert to LE ... > > qla24xx_read_flash_data(vha, dcode, faddr, 4); > > risc_addr = be32_to_cpu(dcode[2]); > *srisc_addr = *srisc_addr == 0 ? risc_addr : *srisc_addr; > risc_size = be32_to_cpu(dcode[3]); > >then happily assume it's BE. > >And again, coming from flash, it's unlikely to actually be in the native >endian-ness as callers seem to assume. I'm guessing it's all BE. > >I poked at it a bit and was able to cut down # of warnings >from 1700 to 1400 in an hour. Someone familiar with the code >should look at it. We’ll take a look and send patches to resolve these warnings. > >-- >MST >_______________________________________________ >Virtualization mailing list >Virtualization@lists.linux-foundation.org >https://lists.linuxfoundation.org/mailman/listinfo/virtualization