From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <507648F5.1030404@st.com> Date: Thu, 11 Oct 2012 09:50:05 +0530 From: Vipin Kumar MIME-Version: 1.0 To: Russell King - ARM Linux Subject: Re: [PATCH 09/11] fsmc/nand:FIX: replace change_bit routine References: <20121010204546.GK4625@n2100.arm.linux.org.uk> In-Reply-To: <20121010204546.GK4625@n2100.arm.linux.org.uk> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: "dedekind1@gmail.com" , Linus Walleij , spear-devel , "linux-mtd@lists.infradead.org" , Nicolas Pitre , "plagnioj@jcrosoft.com" , "linux-arm-kernel@lists.infradead.org" List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 10/11/2012 2:15 AM, Russell King - ARM Linux wrote: > On Wed, Oct 10, 2012 at 07:22:04PM +0200, Linus Walleij wrote: >> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar wrote: >> >>> change_bit routine accepts only ulong pointers as buffer, so an unaligned char >>> pointer passed to change_bit may lead to a crash. >>> >>> Fix this bug by accessing the buffer as char pointer. >> >> Why not see if we can fix change_bit() instead? >> Since I suspect this is on ARM I bet Russell and Nico >> want to hear about this if there is a problem. > > Not particularly. There's a reason the argument to change_bit() is typed > 'unsigned long' and that's not because it can take a void, char, or a > short. It's because it _expects_ the buffer to be aligned to an > "unsigned long" quantity. > > That's because on many architectures, misaligned loads and stores are > not atomic operations - and in this case, load/store exclusive will > fail when they're misalighed. > > So... > > - change_bit(err_idx[i], (unsigned long *)dat); > > is highly invalid code. > Thanks. Got your point >> Can the ARM change_bit() not be fixed, so that >> long arguments are the only option? > > Spot the intentional cast: > >>> - change_bit(err_idx[i], (unsigned long *)dat); > > which tries to work around this. Remember my attitude towards casts: > if you're having to use a cast, you are _probably_ doing something > wrong. In this case, it's hiding a warning which was saying that > the code is doing something wrong, and then the result blows up. > By adding that cast, the wrong wire was cut... you get to keep the > remains. ;) > . I agree with you. Although I was a bit relaxed this time, I would really think before adding a cast explicitly just to avoid a warning -Vipin From mboxrd@z Thu Jan 1 00:00:00 1970 From: vipin.kumar@st.com (Vipin Kumar) Date: Thu, 11 Oct 2012 09:50:05 +0530 Subject: [PATCH 09/11] fsmc/nand:FIX: replace change_bit routine In-Reply-To: <20121010204546.GK4625@n2100.arm.linux.org.uk> References: <20121010204546.GK4625@n2100.arm.linux.org.uk> Message-ID: <507648F5.1030404@st.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 10/11/2012 2:15 AM, Russell King - ARM Linux wrote: > On Wed, Oct 10, 2012 at 07:22:04PM +0200, Linus Walleij wrote: >> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar wrote: >> >>> change_bit routine accepts only ulong pointers as buffer, so an unaligned char >>> pointer passed to change_bit may lead to a crash. >>> >>> Fix this bug by accessing the buffer as char pointer. >> >> Why not see if we can fix change_bit() instead? >> Since I suspect this is on ARM I bet Russell and Nico >> want to hear about this if there is a problem. > > Not particularly. There's a reason the argument to change_bit() is typed > 'unsigned long' and that's not because it can take a void, char, or a > short. It's because it _expects_ the buffer to be aligned to an > "unsigned long" quantity. > > That's because on many architectures, misaligned loads and stores are > not atomic operations - and in this case, load/store exclusive will > fail when they're misalighed. > > So... > > - change_bit(err_idx[i], (unsigned long *)dat); > > is highly invalid code. > Thanks. Got your point >> Can the ARM change_bit() not be fixed, so that >> long arguments are the only option? > > Spot the intentional cast: > >>> - change_bit(err_idx[i], (unsigned long *)dat); > > which tries to work around this. Remember my attitude towards casts: > if you're having to use a cast, you are _probably_ doing something > wrong. In this case, it's hiding a warning which was saying that > the code is doing something wrong, and then the result blows up. > By adding that cast, the wrong wire was cut... you get to keep the > remains. ;) > . I agree with you. Although I was a bit relaxed this time, I would really think before adding a cast explicitly just to avoid a warning -Vipin