diff mbox series

[net-next,4/4] net: ipa: activate some commented assertions

Message ID 20210319042923.1584593-5-elder@linaro.org
State New
Headers show
Series net: ipa: fix validation | expand

Commit Message

Alex Elder March 19, 2021, 4:29 a.m. UTC 
Convert some commented assertion statements into real calls to
ipa_assert().  If the IPA device pointer is available, provide it,
otherwise pass NULL for that.

There are lots more places to convert, but this serves as an initial
verification of the new mechanism.  The assertions here implement
both runtime and build-time assertions, both with and without the
device pointer.

Signed-off-by: Alex Elder <elder@linaro.org>

---
 drivers/net/ipa/ipa_reg.h   | 7 ++++---
 drivers/net/ipa/ipa_table.c | 5 ++++-
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.27.0

Comments

Leon Romanovsky March 19, 2021, 5 a.m. UTC | #1 
On Thu, Mar 18, 2021 at 11:29:23PM -0500, Alex Elder wrote:
> Convert some commented assertion statements into real calls to

> ipa_assert().  If the IPA device pointer is available, provide it,

> otherwise pass NULL for that.

> 

> There are lots more places to convert, but this serves as an initial

> verification of the new mechanism.  The assertions here implement

> both runtime and build-time assertions, both with and without the

> device pointer.

> 

> Signed-off-by: Alex Elder <elder@linaro.org>

> ---

>  drivers/net/ipa/ipa_reg.h   | 7 ++++---

>  drivers/net/ipa/ipa_table.c | 5 ++++-

>  2 files changed, 8 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h

> index 732e691e9aa62..d0de85de9f08d 100644

> --- a/drivers/net/ipa/ipa_reg.h

> +++ b/drivers/net/ipa/ipa_reg.h

> @@ -9,6 +9,7 @@

>  #include <linux/bitfield.h>

>  

>  #include "ipa_version.h"

> +#include "ipa_assert.h"

>  

>  struct ipa;

>  

> @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)

>  			BCR_HOLB_DROP_L2_IRQ_FMASK |

>  			BCR_DUAL_TX_FMASK;

>  

> -	/* assert(version != IPA_VERSION_4_5); */

> +	ipa_assert(NULL, version != IPA_VERSION_4_5);


This assert will fire for IPA_VERSION_4_2, I doubt that this is
something you want.

Thanks
Alex Elder March 19, 2021, 12:40 p.m. UTC | #2 
On 3/19/21 12:00 AM, Leon Romanovsky wrote:
> On Thu, Mar 18, 2021 at 11:29:23PM -0500, Alex Elder wrote:

>> Convert some commented assertion statements into real calls to

>> ipa_assert().  If the IPA device pointer is available, provide it,

>> otherwise pass NULL for that.

>>

>> There are lots more places to convert, but this serves as an initial

>> verification of the new mechanism.  The assertions here implement

>> both runtime and build-time assertions, both with and without the

>> device pointer.

>>

>> Signed-off-by: Alex Elder <elder@linaro.org>

>> ---

>>   drivers/net/ipa/ipa_reg.h   | 7 ++++---

>>   drivers/net/ipa/ipa_table.c | 5 ++++-

>>   2 files changed, 8 insertions(+), 4 deletions(-)

>>

>> diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h

>> index 732e691e9aa62..d0de85de9f08d 100644

>> --- a/drivers/net/ipa/ipa_reg.h

>> +++ b/drivers/net/ipa/ipa_reg.h

>> @@ -9,6 +9,7 @@

>>   #include <linux/bitfield.h>

>>   

>>   #include "ipa_version.h"

>> +#include "ipa_assert.h"

>>   

>>   struct ipa;

>>   

>> @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)

>>   			BCR_HOLB_DROP_L2_IRQ_FMASK |

>>   			BCR_DUAL_TX_FMASK;

>>   

>> -	/* assert(version != IPA_VERSION_4_5); */

>> +	ipa_assert(NULL, version != IPA_VERSION_4_5);

> 

> This assert will fire for IPA_VERSION_4_2, I doubt that this is

> something you want.


No, it will only fail if version == IPA_VERSION_4_5.
The logic of an assertion is the opposite of BUG_ON().
It fails only if the asserted condition yields false.

					-Alex

> 

> Thanks

>
Leon Romanovsky March 19, 2021, 3:17 p.m. UTC | #3 
On Fri, Mar 19, 2021 at 07:40:21AM -0500, Alex Elder wrote:
> On 3/19/21 12:00 AM, Leon Romanovsky wrote:

> > On Thu, Mar 18, 2021 at 11:29:23PM -0500, Alex Elder wrote:

> > > Convert some commented assertion statements into real calls to

> > > ipa_assert().  If the IPA device pointer is available, provide it,

> > > otherwise pass NULL for that.

> > > 

> > > There are lots more places to convert, but this serves as an initial

> > > verification of the new mechanism.  The assertions here implement

> > > both runtime and build-time assertions, both with and without the

> > > device pointer.

> > > 

> > > Signed-off-by: Alex Elder <elder@linaro.org>

> > > ---

> > >   drivers/net/ipa/ipa_reg.h   | 7 ++++---

> > >   drivers/net/ipa/ipa_table.c | 5 ++++-

> > >   2 files changed, 8 insertions(+), 4 deletions(-)

> > > 

> > > diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h

> > > index 732e691e9aa62..d0de85de9f08d 100644

> > > --- a/drivers/net/ipa/ipa_reg.h

> > > +++ b/drivers/net/ipa/ipa_reg.h

> > > @@ -9,6 +9,7 @@

> > >   #include <linux/bitfield.h>

> > >   #include "ipa_version.h"

> > > +#include "ipa_assert.h"

> > >   struct ipa;

> > > @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)

> > >   			BCR_HOLB_DROP_L2_IRQ_FMASK |

> > >   			BCR_DUAL_TX_FMASK;

> > > -	/* assert(version != IPA_VERSION_4_5); */

> > > +	ipa_assert(NULL, version != IPA_VERSION_4_5);

> > 

> > This assert will fire for IPA_VERSION_4_2, I doubt that this is

> > something you want.

> 

> No, it will only fail if version == IPA_VERSION_4_5.

> The logic of an assertion is the opposite of BUG_ON().

> It fails only if the asserted condition yields false.


ok, this ipa_reg_bcr_val() is called in only one place and has a
protection from IPA_VERSION_4_5, why don't you code it at the same
.c file instead of adding useless assert?

> 

> 					-Alex

> 

> > 

> > Thanks

> > 

>
Alex Elder March 19, 2021, 3:32 p.m. UTC | #4 
On 3/19/21 10:17 AM, Leon Romanovsky wrote:
>>>> @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)

>>>>    			BCR_HOLB_DROP_L2_IRQ_FMASK |

>>>>    			BCR_DUAL_TX_FMASK;

>>>> -	/* assert(version != IPA_VERSION_4_5); */

>>>> +	ipa_assert(NULL, version != IPA_VERSION_4_5);

>>> This assert will fire for IPA_VERSION_4_2, I doubt that this is

>>> something you want.

>> No, it will only fail if version == IPA_VERSION_4_5.

>> The logic of an assertion is the opposite of BUG_ON().

>> It fails only if the asserted condition yields false.

> ok, this ipa_reg_bcr_val() is called in only one place and has a

> protection from IPA_VERSION_4_5, why don't you code it at the same

> .c file instead of adding useless assert?


As I mentioned in my other message, the purpose of an
assertion is *communicating with the reader*.  The
fact that an assertion may expand to code that ensures
the assertion is true is secondary.

This particular assertion says that the version will never
be 4.5.  While looking at this function, you don't need to
see if the caller ensures that, the assertion *tells* you.

Whether an assertion is warranted is really subjective.
You may not appreciate the value of that, but I do.

					-Alex
Andrew Lunn March 19, 2021, 6:32 p.m. UTC | #5 
> @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)

>  			BCR_HOLB_DROP_L2_IRQ_FMASK |

>  			BCR_DUAL_TX_FMASK;

>  

> -	/* assert(version != IPA_VERSION_4_5); */

> +	ipa_assert(NULL, version != IPA_VERSION_4_5);


Hi Alex

It is impossible to see just looking what the NULL means. And given
its the first parameter, it should be quite important. I find this API
pretty bad.

    Andrew
Alex Elder March 19, 2021, 9:18 p.m. UTC | #6 
On 3/19/21 1:32 PM, Andrew Lunn wrote:
>> @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)

>>   			BCR_HOLB_DROP_L2_IRQ_FMASK |

>>   			BCR_DUAL_TX_FMASK;

>>   

>> -	/* assert(version != IPA_VERSION_4_5); */

>> +	ipa_assert(NULL, version != IPA_VERSION_4_5);

> 

> Hi Alex

> 

> It is impossible to see just looking what the NULL means. And given

> its the first parameter, it should be quite important. I find this API

> pretty bad.


I actually don't like the first argument.  I would have
supplied the main IPA pointer, but that isn't always
visible or available (the GSI code doesn't have the
IPA pointer definition).  So I thought instead I could
at least supply the underlying device if available,
and use dev_err().

But I wouldn't mind just getting rid of the first
argument and having a failed assertion always call
pr_err() and not dev_err().

The thing of most value to me the asserted condition.

Thoughts?

					-Alex
Andrew Lunn March 19, 2021, 9:30 p.m. UTC | #7 
On Fri, Mar 19, 2021 at 04:18:32PM -0500, Alex Elder wrote:
> On 3/19/21 1:32 PM, Andrew Lunn wrote:

> > > @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)

> > >   			BCR_HOLB_DROP_L2_IRQ_FMASK |

> > >   			BCR_DUAL_TX_FMASK;

> > > -	/* assert(version != IPA_VERSION_4_5); */

> > > +	ipa_assert(NULL, version != IPA_VERSION_4_5);

> > 

> > Hi Alex

> > 

> > It is impossible to see just looking what the NULL means. And given

> > its the first parameter, it should be quite important. I find this API

> > pretty bad.

> 

> I actually don't like the first argument.  I would have

> supplied the main IPA pointer, but that isn't always

> visible or available (the GSI code doesn't have the

> IPA pointer definition).  So I thought instead I could

> at least supply the underlying device if available,

> and use dev_err().

> 

> But I wouldn't mind just getting rid of the first

> argument and having a failed assertion always call

> pr_err() and not dev_err().

> 

> The thing of most value to me the asserted condition.


Hi Alex

What you really want to be looking at is adding a
WARN_ON_DEV(dev, condition) macro.

	 Andrew
diff mbox series

Patch

diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
index 732e691e9aa62..d0de85de9f08d 100644
--- a/drivers/net/ipa/ipa_reg.h
+++ b/drivers/net/ipa/ipa_reg.h
@@ -9,6 +9,7 @@ 
 #include <linux/bitfield.h>
 
 #include "ipa_version.h"
+#include "ipa_assert.h"
 
 struct ipa;
 
@@ -212,7 +213,7 @@  static inline u32 ipa_reg_bcr_val(enum ipa_version version)
 			BCR_HOLB_DROP_L2_IRQ_FMASK |
 			BCR_DUAL_TX_FMASK;
 
-	/* assert(version != IPA_VERSION_4_5); */
+	ipa_assert(NULL, version != IPA_VERSION_4_5);
 
 	return 0x00000000;
 }
@@ -413,7 +414,7 @@  static inline u32 ipa_header_size_encoded(enum ipa_version version,
 
 	val = u32_encode_bits(size, HDR_LEN_FMASK);
 	if (version < IPA_VERSION_4_5) {
-		/* ipa_assert(header_size == size); */
+		ipa_assert(NULL, header_size == size);
 		return val;
 	}
 
@@ -433,7 +434,7 @@  static inline u32 ipa_metadata_offset_encoded(enum ipa_version version,
 
 	val = u32_encode_bits(off, HDR_OFST_METADATA_FMASK);
 	if (version < IPA_VERSION_4_5) {
-		/* ipa_assert(offset == off); */
+		ipa_assert(NULL, offset == off);
 		return val;
 	}
 
diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index aa8b3ce7e21d9..7784b42fbaccc 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -23,6 +23,7 @@ 
 #include "ipa_cmd.h"
 #include "gsi.h"
 #include "gsi_trans.h"
+#include "ipa_assert.h"
 
 /**
  * DOC: IPA Filter and Route Tables
@@ -237,11 +238,13 @@  static void ipa_table_validate_build(void)
 static dma_addr_t ipa_table_addr(struct ipa *ipa, bool filter_mask, u16 count)
 {
 	u32 skip;
+	u32 max;
 
 	if (!count)
 		return 0;
 
-/* assert(count <= max_t(u32, IPA_FILTER_COUNT_MAX, IPA_ROUTE_COUNT_MAX)); */
+	max = max_t(u32, IPA_FILTER_COUNT_MAX, IPA_ROUTE_COUNT_MAX);
+	ipa_assert(&ipa->pdev->dev, max);
 
 	/* Skip over the zero rule and possibly the filter mask */
 	skip = filter_mask ? 1 : 2;