gpu: nova-core: fix three clippy::precedence warnings

[PATCH] gpu: nova-core: fix three clippy::precedence warnings

Posted by John Hubbard 2 months, 1 week ago

Clippy warns when shifts and bitwise-OR are mixed without parentheses,
because the relative precedence of << and | is easy to misread. This was
causing 3 warnings in drm-rust-next.

For read_sysmem_flush_page_ga100(), add explicit parentheses.

For the PCI ROM header parsing, replace the manual byte-shifting with
u32::from_le_bytes(), which both fixes the warning and is clearer about
the intended byte order.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/fb/hal/ga100.rs | 7 ++++---
 drivers/gpu/nova-core/vbios.rs        | 7 +------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
index 1c03783cddef..b5d0f04198f0 100644
--- a/drivers/gpu/nova-core/fb/hal/ga100.rs
+++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
@@ -17,9 +17,10 @@
 struct Ga100;
 
 pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> u64 {
-    u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR).adr_39_08()) << FLUSH_SYSMEM_ADDR_SHIFT
-        | u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI).adr_63_40())
-            << FLUSH_SYSMEM_ADDR_SHIFT_HI
+    (u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR).adr_39_08())
+        << FLUSH_SYSMEM_ADDR_SHIFT)
+        | (u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI).adr_63_40())
+            << FLUSH_SYSMEM_ADDR_SHIFT_HI)
 }
 
 pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: u64) {
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 3e3fa5b72524..ebda28e596c5 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -507,12 +507,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
 
         if data.len() >= 30 {
             // Read size_of_block at offset 0x1A.
-            size_of_block = Some(
-                u32::from(data[29]) << 24
-                    | u32::from(data[28]) << 16
-                    | u32::from(data[27]) << 8
-                    | u32::from(data[26]),
-            );
+            size_of_block = Some(u32::from_le_bytes([data[26], data[27], data[28], data[29]]));
         }
 
         // For NBSI images, try to read the nbsiDataOffset at offset 0x16.

base-commit: 7c50d748b4a635bc39802ea3f6b120e66b1b9067
-- 
2.53.0

Re: [PATCH] gpu: nova-core: fix three clippy::precedence warnings

Posted by Miguel Ojeda 2 months, 1 week ago

On Sat, Apr 4, 2026 at 4:58 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> Clippy warns when shifts and bitwise-OR are mixed without parentheses,
> because the relative precedence of << and | is easy to misread. This was
> causing 3 warnings in drm-rust-next.
>
> For read_sysmem_flush_page_ga100(), add explicit parentheses.
>
> For the PCI ROM header parsing, replace the manual byte-shifting with
> u32::from_le_bytes(), which both fixes the warning and is clearer about
> the intended byte order.
>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

Please see:

  https://lore.kernel.org/rust-for-linux/20260401114540.30108-34-ojeda@kernel.org/

i.e. the plan was to allow it, since it may be a bit too annoying for
some. It was also the reason it was split upstream into a new lint,
which is why you only see it in Rust 1.85.0:

  The Clippy `precedence` lint was extended in Rust 1.85.0 to include
  bitmasking and shift operations [1]. However, because it generated
  many hits, in Rust 1.86.0 it was split into a new `precedence_bits`
  lint which is not enabled by default [2].

  In other words, only Rust 1.85 has a different behavior.

Having said that, if people prefer to have the extra parenthesis in
that case, then I don't mind to keep it enabled.

Thanks!

Cheers,
Miguel

Re: [PATCH] gpu: nova-core: fix three clippy::precedence warnings

Posted by Danilo Krummrich 2 months, 1 week ago

On Sat Apr 4, 2026 at 1:13 PM CEST, Miguel Ojeda wrote:
> On Sat, Apr 4, 2026 at 4:58 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> Clippy warns when shifts and bitwise-OR are mixed without parentheses,
>> because the relative precedence of << and | is easy to misread. This was
>> causing 3 warnings in drm-rust-next.
>>
>> For read_sysmem_flush_page_ga100(), add explicit parentheses.
>>
>> For the PCI ROM header parsing, replace the manual byte-shifting with
>> u32::from_le_bytes(), which both fixes the warning and is clearer about
>> the intended byte order.
>>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>
> Please see:
>
>   https://lore.kernel.org/rust-for-linux/20260401114540.30108-34-ojeda@kernel.org/
>
> i.e. the plan was to allow it, since it may be a bit too annoying for
> some. It was also the reason it was split upstream into a new lint,
> which is why you only see it in Rust 1.85.0:
>
>   The Clippy `precedence` lint was extended in Rust 1.85.0 to include
>   bitmasking and shift operations [1]. However, because it generated
>   many hits, in Rust 1.86.0 it was split into a new `precedence_bits`
>   lint which is not enabled by default [2].
>
>   In other words, only Rust 1.85 has a different behavior.
>
> Having said that, if people prefer to have the extra parenthesis in
> that case, then I don't mind to keep it enabled.

I don't mind having the extra parenthesis, but I think we should disable the
lint regardless, as I think it is a bit too much.

I also think we should drop this patch, as it is not worth the churn, if we
disable the lint.

Thanks,
Danilo

Re: [PATCH] gpu: nova-core: fix three clippy::precedence warnings

Posted by John Hubbard 2 months, 1 week ago

On 4/4/26 4:29 AM, Danilo Krummrich wrote:
> On Sat Apr 4, 2026 at 1:13 PM CEST, Miguel Ojeda wrote:
>> On Sat, Apr 4, 2026 at 4:58 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>>
>>> Clippy warns when shifts and bitwise-OR are mixed without parentheses,
>>> because the relative precedence of << and | is easy to misread. This was
>>> causing 3 warnings in drm-rust-next.
>>>
>>> For read_sysmem_flush_page_ga100(), add explicit parentheses.
>>>
>>> For the PCI ROM header parsing, replace the manual byte-shifting with
>>> u32::from_le_bytes(), which both fixes the warning and is clearer about
>>> the intended byte order.
>>>
>>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>>
>> Please see:
>>
>>   https://lore.kernel.org/rust-for-linux/20260401114540.30108-34-ojeda@kernel.org/
>>
>> i.e. the plan was to allow it, since it may be a bit too annoying for
>> some. It was also the reason it was split upstream into a new lint,
>> which is why you only see it in Rust 1.85.0:
>>
>>   The Clippy `precedence` lint was extended in Rust 1.85.0 to include
>>   bitmasking and shift operations [1]. However, because it generated
>>   many hits, in Rust 1.86.0 it was split into a new `precedence_bits`
>>   lint which is not enabled by default [2].
>>
>>   In other words, only Rust 1.85 has a different behavior.
>>
>> Having said that, if people prefer to have the extra parenthesis in
>> that case, then I don't mind to keep it enabled.
> 
> I don't mind having the extra parenthesis, but I think we should disable the
> lint regardless, as I think it is a bit too much.
> 
> I also think we should drop this patch, as it is not worth the churn, if we
> disable the lint.

OK, I sent v2 out before I noticed this, but I agree. :)

thanks,
-- 
John Hubbard

Re: [PATCH] gpu: nova-core: fix three clippy::precedence warnings

Posted by Alexandre Courbot 2 months, 1 week ago

On Sat Apr 4, 2026 at 11:58 AM JST, John Hubbard wrote:
> Clippy warns when shifts and bitwise-OR are mixed without parentheses,
> because the relative precedence of << and | is easy to misread. This was
> causing 3 warnings in drm-rust-next.

These don't manifest for me (rustc 1.94.1), can you share more about the
build context?

>
> For read_sysmem_flush_page_ga100(), add explicit parentheses.
>
> For the PCI ROM header parsing, replace the manual byte-shifting with
> u32::from_le_bytes(), which both fixes the warning and is clearer about
> the intended byte order.
>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  drivers/gpu/nova-core/fb/hal/ga100.rs | 7 ++++---
>  drivers/gpu/nova-core/vbios.rs        | 7 +------
>  2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
> index 1c03783cddef..b5d0f04198f0 100644
> --- a/drivers/gpu/nova-core/fb/hal/ga100.rs
> +++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
> @@ -17,9 +17,10 @@
>  struct Ga100;
>  
>  pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> u64 {
> -    u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR).adr_39_08()) << FLUSH_SYSMEM_ADDR_SHIFT
> -        | u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI).adr_63_40())
> -            << FLUSH_SYSMEM_ADDR_SHIFT_HI
> +    (u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR).adr_39_08())
> +        << FLUSH_SYSMEM_ADDR_SHIFT)
> +        | (u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI).adr_63_40())
> +            << FLUSH_SYSMEM_ADDR_SHIFT_HI)
>  }
>  
>  pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: u64) {
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 3e3fa5b72524..ebda28e596c5 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -507,12 +507,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>  
>          if data.len() >= 30 {
>              // Read size_of_block at offset 0x1A.
> -            size_of_block = Some(
> -                u32::from(data[29]) << 24
> -                    | u32::from(data[28]) << 16
> -                    | u32::from(data[27]) << 8
> -                    | u32::from(data[26]),
> -            );
> +            size_of_block = Some(u32::from_le_bytes([data[26], data[27], data[28], data[29]]));

This chunk looks like it should be its own patch as it is valuable
regardless of the clippy warnings.

Re: [PATCH] gpu: nova-core: fix three clippy::precedence warnings

Posted by John Hubbard 2 months, 1 week ago

On 4/3/26 10:45 PM, Alexandre Courbot wrote:
> On Sat Apr 4, 2026 at 11:58 AM JST, John Hubbard wrote:
>> Clippy warns when shifts and bitwise-OR are mixed without parentheses,
>> because the relative precedence of << and | is easy to misread. This was
>> causing 3 warnings in drm-rust-next.
> 
> These don't manifest for me (rustc 1.94.1), can you share more about the
> build context?

rustc 1.85.0

> 
>>
>> For read_sysmem_flush_page_ga100(), add explicit parentheses.
>>
>> For the PCI ROM header parsing, replace the manual byte-shifting with
>> u32::from_le_bytes(), which both fixes the warning and is clearer about
>> the intended byte order.
>>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   drivers/gpu/nova-core/fb/hal/ga100.rs | 7 ++++---
>>   drivers/gpu/nova-core/vbios.rs        | 7 +------
>>   2 files changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
>> index 1c03783cddef..b5d0f04198f0 100644
>> --- a/drivers/gpu/nova-core/fb/hal/ga100.rs
>> +++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
>> @@ -17,9 +17,10 @@
>>   struct Ga100;
>>   
>>   pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> u64 {
>> -    u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR).adr_39_08()) << FLUSH_SYSMEM_ADDR_SHIFT
>> -        | u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI).adr_63_40())
>> -            << FLUSH_SYSMEM_ADDR_SHIFT_HI
>> +    (u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR).adr_39_08())
>> +        << FLUSH_SYSMEM_ADDR_SHIFT)
>> +        | (u64::from(bar.read(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI).adr_63_40())
>> +            << FLUSH_SYSMEM_ADDR_SHIFT_HI)
>>   }
>>   
>>   pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: u64) {
>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>> index 3e3fa5b72524..ebda28e596c5 100644
>> --- a/drivers/gpu/nova-core/vbios.rs
>> +++ b/drivers/gpu/nova-core/vbios.rs
>> @@ -507,12 +507,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>>   
>>           if data.len() >= 30 {
>>               // Read size_of_block at offset 0x1A.
>> -            size_of_block = Some(
>> -                u32::from(data[29]) << 24
>> -                    | u32::from(data[28]) << 16
>> -                    | u32::from(data[27]) << 8
>> -                    | u32::from(data[26]),
>> -            );
>> +            size_of_block = Some(u32::from_le_bytes([data[26], data[27], data[28], data[29]]));
> 
> This chunk looks like it should be its own patch as it is valuable
> regardless of the clippy warnings.

OK.

thanks,
-- 
John Hubbard