From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 63242C433FE for ; Mon, 3 Oct 2022 08:59:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:Subject:From:References:To:MIME-Version: Date:Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=ZycmRfQiHpsgeVmOeJ+8Ju1TBL9GNnOQ/CYJvR2HeFY=; b=Dmpn5n/NMnP27MH6Hjm3I9sFy4 yVrKVnoE9MzL+BaF17TBE3fCdMFGHTKryyIruNkpeduQPSk18bB0MfhdMNKf3EoUetmrgwWsSJoxN cA5DKdTHjS7c5ghypefoc+Uydy63UAtvSzSe55etkJzO+ztG3cC3gXm/OHOoLia/B7N616cHs3boA T95aX14ub1pbpFZFOQG/FdfHTjaUurJ3AWAzcRBaUZ8GeolwbenDogLQjujyz9PLPiPH1d5NB5y+X k3VOOWn/vZETGaLWBG1xOMm8gOkmtIZ2wkpqsLEreoVm8Hq5XbdVRHt+4JGhyHsaeHfRMqpIRSmgf BIYHREVQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ofHHf-004qx2-69; Mon, 03 Oct 2022 08:58:43 +0000 Received: from mail-lf1-x12b.google.com ([2a00:1450:4864:20::12b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ofHHc-004qvT-41 for linux-arm-kernel@lists.infradead.org; Mon, 03 Oct 2022 08:58:41 +0000 Received: by mail-lf1-x12b.google.com with SMTP id d18so2552614lfb.0 for ; Mon, 03 Oct 2022 01:58:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:subject:from:content-language :references:cc:to:user-agent:mime-version:date:message-id:from:to:cc :subject:date; bh=XqQodRH6a0lQZyrQzj66WYhNicw1wjJoyMwDWMJ/yGw=; b=bl1ytC6xg/0TJjIaMmN+k7HXrHD8sja3rF4SvTIZx7wgGqYO/r+PfPOrhBeKihL4f0 kaO6kuj2Yu6L7uImCcWZ5u9PmECHLQ264Vx031PAxwHI4Q3SUhtNKhCsZJciqhTKabCV pBKnGkX8lrftB2h0lPVg98YEw3Xt2g3gRRrmABN+ZWD2JtdA/sYGPy8vl1F9KHz1GmD+ xCp7P35IhJYRDKtpWBTocD8ZOfapZHSrhC/ZPtoHtVv9CLC4cHahRYMjb/S8VoZlS+xb ZEixKtM/Dy2QNEVJYz1rRHw6OF9FjQGsytLKdYupw5tPH63XrFos3zFIwpPOzf3z3i2Y u06Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:subject:from:content-language :references:cc:to:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=XqQodRH6a0lQZyrQzj66WYhNicw1wjJoyMwDWMJ/yGw=; b=PMb96TAQt4iihhomQooo9qs4gI0evZQjET5HQyr1DDRx8iPKb2mtRWQbSx3LclnPhA LWqn9e3OIj4nGD7m5ZFpNEdbQJl4qmh5Z90+dywKNV2XXN1GWfAXK7QQ7LK9sDWbMDvB S5kJZE+YYYV4g7NCkECtrNue5PKg4ID2jiMooDGWN+Ika98tiJbLqkDxziHv2wAvH9m9 4+IdzwfEXbChjDAqyLhH5NIE4A7okWGejiqUMW2U934Nkzdb8aawQGlXPQoQVYriYC4J yvisMr+AWHCYK9xGu3fJsgpzEsM94qJa3jNRfi0s3Vw3H7QhAa90OCX8KJQxKn8FBoLF l8Hg== X-Gm-Message-State: ACrzQf1zITl3XTxBDH7kOpLHE7LF2m1aieBwHwFvZxUtCRqGIER7kDTr GN//laK2Ierv4IjC4B1j09A= X-Google-Smtp-Source: AMsMyM5iOPWlnaPtqydnwwB96XQWlOM0ceWCm7e8fBeobmzF3gn2nYS9+MkZN4L3bjNU5DHtFJiI0g== X-Received: by 2002:a05:6512:b21:b0:4a0:45a1:7bf1 with SMTP id w33-20020a0565120b2100b004a045a17bf1mr6618049lfu.461.1664787517419; Mon, 03 Oct 2022 01:58:37 -0700 (PDT) Received: from ?IPV6:2001:14ba:16f3:4a00::1? (dc75zzyyyyyyyyyyyyyyt-3.rev.dnainternet.fi. [2001:14ba:16f3:4a00::1]) by smtp.gmail.com with ESMTPSA id o5-20020ac24e85000000b004886508ca5csm1051678lfr.68.2022.10.03.01.58.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 03 Oct 2022 01:58:36 -0700 (PDT) Message-ID: Date: Mon, 3 Oct 2022 11:58:35 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 To: Andy Shevchenko References: <63f54787a684eb1232f1c5d275a09c786987fe4a.1664782676.git.mazziesaccount@gmail.com> Content-Language: en-US From: Matti Vaittinen Subject: Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221003_015840_193759_92C9610B X-CRM114-Status: GOOD ( 30.69 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , linux-iio@vger.kernel.org, Gwendal Grignou , linux-kernel@vger.kernel.org, Paul Cercueil , Miquel Raynal , Guenter Roeck , chrome-platform@lists.linux.dev, Lars-Peter Clausen , Miaoqian Lin , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= , Alexandru Ardelean , Mihail Chindris , Michael Hennerich , Cosmin Tanislav , Nathan Chancellor , Benson Leung , linux-arm-kernel@lists.infradead.org, Matti Vaittinen , Eugen Hristev , Claudiu Beznea , Jonathan Cameron Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Andy, Thanks for taking the time to review :) Much appreciated. On 10/3/22 11:43, Andy Shevchenko wrote: > On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote: >> The iio_triggered_buffer_setup_ext() and the >> devm_iio_kfifo_buffer_setup_ext() were changed by >> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr") >> to silently expect that all attributes given in buffer_attrs array are >> device-attributes. This expectation was not forced by the API - and some >> drivers did register attributes created by IIO_CONST_ATTR(). >> >> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy >> the pointer to stored string constant and when the sysfs file is read the >> kernel will access to invalid location. >> >> Change the function signatures to expect an array of iio_dev_attrs to >> avoid similar errors in the future. > > ... > > > Wouldn't be better to split this on per driver basis or is it impossible? We need to change the callers and function signatures in one patch so we don't break bisecting. > >> struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); >> struct iio_dev_attr *p; > >> + const struct iio_dev_attr *id_attr; > > I'm wondering if we may keep this upper, so "longer line goes first" rule would > be satisfied. Sure. > >> struct attribute **attr; >> int ret, i, attrn, scan_el_attrcount, buffer_attrcount; >> const struct iio_chan_spec *channels; > > ... > >> + for (i = 0, id_attr = buffer->attrs[i]; >> + (id_attr = buffer->attrs[i]); i++) > > Not sure why we have additional parentheses... Because gcc warns about the assignment and suggests adding parenthesis if we don't. >> + attr[ARRAY_SIZE(iio_buffer_attrs) + i] = >> + (struct attribute *)&id_attr->dev_attr.attr; > > ...and explicit casting here. Isn't attr is already of a struct attribute? I am glad you asked :) This is one of the "things" I was not really happy about. Here we hide the fact that our array is full of pointers to _const_ data. If we don't cast the compiler points this out. Old code did the same thing but it did this by just doing a memcpy for the pointers - which I personally consider even worse as it gets really easy to miss this. The cast at least hints there is something slightly "fishy" going on. My "gut feeling" about the correct fix is we should check if some attributes in the array (stored to the struct here) actually need to be modified later (which I doubt). If I was keen on betting I'd bet we could switch the struct definition to also contain pointers to const attributes. I am afraid this would mean quite a few more changes to the function signatures (changing struct attribute * to const struct attribute *) here and there - and possibly also require some changes to drivers. Thus I didn't even look at that option in the scope of this fix. It should probably be a separate refactoring series. But yes - this cast should catch attention as it did. Yours, -- Matti Vaittinen -- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel