Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932621AbcLHQRq (ORCPT ); Thu, 8 Dec 2016 11:17:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60928 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932360AbcLHQRm (ORCPT ); Thu, 8 Dec 2016 11:17:42 -0500 Date: Thu, 8 Dec 2016 18:17:32 +0200 From: "Michael S. Tsirkin" To: Bart Van Assche Cc: "linux-kernel@vger.kernel.org" , Linus Torvalds , Christoph Hellwig , Jason Wang , "linux-kbuild@vger.kernel.org" , Michal Marek , Arnd Bergmann , Greg Kroah-Hartman , Matt Mackall , Herbert Xu , David Airlie , Gerd Hoffmann , Ohad Ben-Cohen , Christian Borntraeger , Cornelia Huck , "James E.J. Bottomley" , "David S. Miller" , Jens Axboe , Neil Armstrong , Stefan Hajnoczi , Asias He , "linux-crypto@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "virtualization@lists.linux-foundation.org" , "netdev@vger.kernel.org" , "linux-remoteproc@vger.kernel.org" , "linux-s390@vger.kernel.org" , "kvm@vger.kernel.org" , "linux-scsi@vger.kernel.org" , "v9fs-developer@lists.sourceforge.net" Subject: Re: [PATCH] linux/types.h: enable endian checks for all sparse builds Message-ID: <20161208163658-mutt-send-email-mst@kernel.org> References: <1481164052-28036-1-git-send-email-mst@redhat.com> <20161208075152-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 08 Dec 2016 16:17:42 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4266 Lines: 127 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. -- MST