Message ID | 20190704081133.7818-2-amitay@ozlabs.org |
---|---|
State | Accepted |
Headers | show |
Series | Improve istep command usability | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (6a4185ca278930693a6edb6d003267f74df3d2e8) |
snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
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) {
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.
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.
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) {
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- src/istep.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)