[1/3] istep: Add checking for istep major/minor numbers
diff mbox series

Message ID 20190704081133.7818-2-amitay@ozlabs.org
State New
Headers show
Series
  • Improve istep command usability
Related show

Checks

Context Check Description
snowpatch_ozlabs/build-multiarch success Test build-multiarch on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (6a4185ca278930693a6edb6d003267f74df3d2e8)

Commit Message

Amitay Isaacs July 4, 2019, 8:11 a.m. UTC
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
---
 src/istep.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

Comments

Alistair Popple July 11, 2019, 5:39 a.m. UTC | #1
Part of me feels like one day we will need a way of over-riding this (say when 
the SBE decides that istep 5.3 is a thing). Can we just print a warning that 
things may fail instead? From memory the SBE will just respond with an error 
if the wrong numbers are specified anyway, so it shouldn't cause a big issue.

I assume there is no way of probing the SBE to find out what isteps it 
supports? Do you think it would make sense to just keep incrementing until we 
get an invalid argument failure from the SBE?

- Alistair

On Thursday, 4 July 2019 6:11:31 PM AEST Amitay Isaacs wrote:
> Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> ---
>  src/istep.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/src/istep.c b/src/istep.c
> index 08a6215..a9acbe7 100644
> --- a/src/istep.c
> +++ b/src/istep.c
> @@ -20,18 +20,40 @@
>  #include "optcmd.h"
>  #include "path.h"
> 
> +struct istep_data {
> +	int major;
> +	int minor_first;
> +	int minor_last;
> +} istep_data[] = {
> +	{ 2, 2, 17 },
> +	{ 3, 1, 22 },
> +	{ 4, 1, 34 },
> +	{ 5, 1,  2 },
> +	{ 0, 0, 0  },
> +};
> +
>  static int istep(uint32_t major, uint32_t minor)
>  {
>  	struct pdbg_target *target;
> -	int count = 0;
> +	int count = 0, i;
> 
>  	if (major < 2 || major > 5) {
>  		fprintf(stderr, "Istep major should be 2 to 5\n");
>  		return 0;
>  	}
> -	if (major == 2 && minor == 1) {
> -		fprintf(stderr, "Istep 2 minor should be > 1\n");
> -		return 0;
> +
> +	for (i=0; istep_data[i].major != 0; i++) {
> +		if (istep_data[i].major != major)
> +			continue;
> +
> +		if (minor < istep_data[i].minor_first ||
> +		    minor > istep_data[i].minor_last) {
> +			fprintf(stderr, "Istep %d minor should be %d to %d\n",
> +				major,
> +				istep_data[i].minor_first,
> +				istep_data[i].minor_last);
> +			return 0;
> +		}
>  	}
> 
>  	for_each_path_target_class("sbefifo", target) {
Amitay Isaacs July 11, 2019, 5:46 a.m. UTC | #2
On Thu, 2019-07-11 at 15:39 +1000, Alistair Popple wrote:
> Part of me feels like one day we will need a way of over-riding this
> (say when 
> the SBE decides that istep 5.3 is a thing). Can we just print a
> warning that 
> things may fail instead? From memory the SBE will just respond with
> an error 
> if the wrong numbers are specified anyway, so it shouldn't cause a
> big issue.
> 
> I assume there is no way of probing the SBE to find out what isteps
> it 
> supports? Do you think it would make sense to just keep incrementing
> until we 
> get an invalid argument failure from the SBE?

No.  The whole SBE chip-op istep interface sucks.  We can't do anything
better.  I will check if they reply with specific error code in case
it's an invalid major/minor number.  If not, then there is no way to
distinguish between failed istep and invalid istep.

> 
> - Alistair
> 
> On Thursday, 4 July 2019 6:11:31 PM AEST Amitay Isaacs wrote:
> > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> > ---
> >  src/istep.c | 30 ++++++++++++++++++++++++++----
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/istep.c b/src/istep.c
> > index 08a6215..a9acbe7 100644
> > --- a/src/istep.c
> > +++ b/src/istep.c
> > @@ -20,18 +20,40 @@
> >  #include "optcmd.h"
> >  #include "path.h"
> > 
> > +struct istep_data {
> > +	int major;
> > +	int minor_first;
> > +	int minor_last;
> > +} istep_data[] = {
> > +	{ 2, 2, 17 },
> > +	{ 3, 1, 22 },
> > +	{ 4, 1, 34 },
> > +	{ 5, 1,  2 },
> > +	{ 0, 0, 0  },
> > +};
> > +
> >  static int istep(uint32_t major, uint32_t minor)
> >  {
> >  	struct pdbg_target *target;
> > -	int count = 0;
> > +	int count = 0, i;
> > 
> >  	if (major < 2 || major > 5) {
> >  		fprintf(stderr, "Istep major should be 2 to 5\n");
> >  		return 0;
> >  	}
> > -	if (major == 2 && minor == 1) {
> > -		fprintf(stderr, "Istep 2 minor should be > 1\n");
> > -		return 0;
> > +
> > +	for (i=0; istep_data[i].major != 0; i++) {
> > +		if (istep_data[i].major != major)
> > +			continue;
> > +
> > +		if (minor < istep_data[i].minor_first ||
> > +		    minor > istep_data[i].minor_last) {
> > +			fprintf(stderr, "Istep %d minor should be %d to
> > %d\n",
> > +				major,
> > +				istep_data[i].minor_first,
> > +				istep_data[i].minor_last);
> > +			return 0;
> > +		}
> >  	}
> > 
> >  	for_each_path_target_class("sbefifo", target) {
> 
> 
> 

Amitay.
Alistair Popple July 11, 2019, 5:47 a.m. UTC | #3
On Thursday, 11 July 2019 3:46:44 PM AEST Amitay Isaacs wrote:
> On Thu, 2019-07-11 at 15:39 +1000, Alistair Popple wrote:
> > Part of me feels like one day we will need a way of over-riding this
> > (say when
> > the SBE decides that istep 5.3 is a thing). Can we just print a
> > warning that
> > things may fail instead? From memory the SBE will just respond with
> > an error
> > if the wrong numbers are specified anyway, so it shouldn't cause a
> > big issue.
> > 
> > I assume there is no way of probing the SBE to find out what isteps
> > it
> > supports? Do you think it would make sense to just keep incrementing
> > until we
> > get an invalid argument failure from the SBE?
> 
> No.  The whole SBE chip-op istep interface sucks.  We can't do anything
> better.  I will check if they reply with specific error code in case
> it's an invalid major/minor number.  If not, then there is no way to
> distinguish between failed istep and invalid istep.

Ok. Maybe we can get that fixed. At least it can't hurt to ask (or send 
patches) :-)

- Alistair
 
> > - Alistair
> > 
> > On Thursday, 4 July 2019 6:11:31 PM AEST Amitay Isaacs wrote:
> > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> > > ---
> > > 
> > >  src/istep.c | 30 ++++++++++++++++++++++++++----
> > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/istep.c b/src/istep.c
> > > index 08a6215..a9acbe7 100644
> > > --- a/src/istep.c
> > > +++ b/src/istep.c
> > > @@ -20,18 +20,40 @@
> > > 
> > >  #include "optcmd.h"
> > >  #include "path.h"
> > > 
> > > +struct istep_data {
> > > +	int major;
> > > +	int minor_first;
> > > +	int minor_last;
> > > +} istep_data[] = {
> > > +	{ 2, 2, 17 },
> > > +	{ 3, 1, 22 },
> > > +	{ 4, 1, 34 },
> > > +	{ 5, 1,  2 },
> > > +	{ 0, 0, 0  },
> > > +};
> > > +
> > > 
> > >  static int istep(uint32_t major, uint32_t minor)
> > >  {
> > >  
> > >  	struct pdbg_target *target;
> > > 
> > > -	int count = 0;
> > > +	int count = 0, i;
> > > 
> > >  	if (major < 2 || major > 5) {
> > >  	
> > >  		fprintf(stderr, "Istep major should be 2 to 5\n");
> > >  		return 0;
> > >  	
> > >  	}
> > > 
> > > -	if (major == 2 && minor == 1) {
> > > -		fprintf(stderr, "Istep 2 minor should be > 1\n");
> > > -		return 0;
> > > +
> > > +	for (i=0; istep_data[i].major != 0; i++) {
> > > +		if (istep_data[i].major != major)
> > > +			continue;
> > > +
> > > +		if (minor < istep_data[i].minor_first ||
> > > +		    minor > istep_data[i].minor_last) {
> > > +			fprintf(stderr, "Istep %d minor should be %d to
> > > %d\n",
> > > +				major,
> > > +				istep_data[i].minor_first,
> > > +				istep_data[i].minor_last);
> > > +			return 0;
> > > +		}
> > > 
> > >  	}
> > >  	
> > >  	for_each_path_target_class("sbefifo", target) {
> 
> Amitay.

Patch
diff mbox series

diff --git a/src/istep.c b/src/istep.c
index 08a6215..a9acbe7 100644
--- a/src/istep.c
+++ b/src/istep.c
@@ -20,18 +20,40 @@ 
 #include "optcmd.h"
 #include "path.h"
 
+struct istep_data {
+	int major;
+	int minor_first;
+	int minor_last;
+} istep_data[] = {
+	{ 2, 2, 17 },
+	{ 3, 1, 22 },
+	{ 4, 1, 34 },
+	{ 5, 1,  2 },
+	{ 0, 0, 0  },
+};
+
 static int istep(uint32_t major, uint32_t minor)
 {
 	struct pdbg_target *target;
-	int count = 0;
+	int count = 0, i;
 
 	if (major < 2 || major > 5) {
 		fprintf(stderr, "Istep major should be 2 to 5\n");
 		return 0;
 	}
-	if (major == 2 && minor == 1) {
-		fprintf(stderr, "Istep 2 minor should be > 1\n");
-		return 0;
+
+	for (i=0; istep_data[i].major != 0; i++) {
+		if (istep_data[i].major != major)
+			continue;
+
+		if (minor < istep_data[i].minor_first ||
+		    minor > istep_data[i].minor_last) {
+			fprintf(stderr, "Istep %d minor should be %d to %d\n",
+				major,
+				istep_data[i].minor_first,
+				istep_data[i].minor_last);
+			return 0;
+		}
 	}
 
 	for_each_path_target_class("sbefifo", target) {