Received-SPF: pass (google.com: domain of linux-kernel+bounces-31125-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 19 Jan 2024 13:06:25 +0100
From: Niklas Cassel <cassel@kernel.org>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Niklas Cassel <Niklas.Cassel@wdc.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>
Subject: Re: [PATCH] PCI: dwc: Fix a 64bit bug in dw_pcie_ep_raise_msix_irq()
Message-ID: <ZaplwRta8wqczSzW@x1-carbon>
References: <3f9f779c-a32f-4925-9ff9-a706861d3357@moroto.mountain>
 <ZahE455neE3wPnHA@x1-carbon>
 <501533ad-7671-46aa-a034-91e0a6322e6c@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <501533ad-7671-46aa-a034-91e0a6322e6c@moroto.mountain>

On Fri, Jan 19, 2024 at 11:25:51AM +0300, Dan Carpenter wrote:
> On Wed, Jan 17, 2024 at 09:21:41PM +0000, Niklas Cassel wrote:
> > Hello Dan,
> > 
> > On Wed, Jan 17, 2024 at 09:32:08PM +0300, Dan Carpenter wrote:
> > > The "msg_addr" variable is u64.  However, the "tbl_offset" is an unsigned
> > 
> > Here you write tbl_offset.
> > 
> > > int.  This means that when the code does
> > > 
> > > 	msg_addr &= ~aligned_offset;
> > > 
> > > it will unintentionally zero out the high 32 bits.  Declare "tbl_offset"
> > 
> > Here you also write tbl_offset.
> > 
> 
> That's so weird...  I can't imagine how that happened.  Do you think it
> could be a Welsh mice situation where forest creatures are changing my
> work when I'm away from my desk?  https://www.youtube.com/shorts/h8gkIbtaaek

Yes, that it the most likely scenario :)

In fact, I think that is what happened with my original patch too...

Because while the C-standards says:

"""
6.5.3.3 Unary arithmetic operators
The result of the unary - operator is the negative of its (promoted) operand. The integer
promotions are performed on the operand, and the result has the promoted type.
"""


Of course, I also remember that implicit integer promotions are only up to
"int" or "unsigned int".

Because of course it is fine to convert types smaller than int implicitly...
but bigger than int? No way! :)


#include <stdio.h>
#include <stdint.h>

void main()
{
        uint16_t val_16 = 0xffff;
        uint8_t mask_8 = 0xf0;

        uint32_t val_32 = 0xffffffff;
        uint16_t mask_16 = 0x00f0;

        uint64_t val_64 = 0xffffffffffffffff;
        uint32_t mask_32 = 0x000000f0;

        uint16_t res_16 = val_16 & ~mask_8;
        uint32_t res_32 = val_32 & ~mask_16;
        uint64_t res_64 = val_64 & ~mask_32;

        printf("16: res: %#llx val: %#llx mask: %#llx\n", res_16, val_16, mask_8);
        printf("32: res: %#llx val: %#llx mask: %#llx\n", res_32, val_32, mask_16);
        printf("64: res: %#llx val: %#llx mask: %#llx\n", res_64, val_64, mask_32);
}

output:
16: res: 0xff0f val: 0xffff mask: 0xf0
32: res: 0xffffff0f val: 0xffffffff mask: 0xf0
64: res: 0xffffff0f val: 0xffffffffffffffff mask: 0xf0

(Silly me for not also reading 6.3.1.1...)


Kind regards,
Niklas