| Message ID | 20200526051226.70904-6-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make kernel fsi driver generic | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (86851b290ac1771b9a6fb0d5238ebf459ea80a21) |
| snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
On Tue, 26 May 2020 at 05:13, Amitay Isaacs <amitay@ozlabs.org> wrote: > > On rescan, kernel driver re-creates all the slave devices. This > invalidates all the slave deviced opened before the scan. > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > --- > libpdbg/kernel.c | 37 +++++++++++++++++++++---------------- > 1 file changed, 21 insertions(+), 16 deletions(-) > > diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c > index 2cc81f2..912895a 100644 > --- a/libpdbg/kernel.c > +++ b/libpdbg/kernel.c > @@ -116,6 +116,15 @@ static void kernel_fsi_scan_devices(void) > const char *kernel_path = kernel_get_fsi_path(); > char *path; > int rc, fd; > + static bool scanned = false; > + > + /* > + * On fsi bus rescan, kernel re-creates all the slave device entries. > + * It means any currently open devices will be invalid and need to be > + * re-opened. So avoid scanning multiple times. > + */ > + if (scanned) > + return; > > if (!kernel_path) > return; > @@ -139,12 +148,13 @@ static void kernel_fsi_scan_devices(void) > > free(path); > close(fd); > + > + scanned = true; > } > > int kernel_fsi_probe(struct pdbg_target *target) > { > struct fsi *fsi = target_to_fsi(target); > - int tries = 5; > int rc; > const char *kernel_path = kernel_get_fsi_path(); > const char *fsi_path; > @@ -162,24 +172,19 @@ int kernel_fsi_probe(struct pdbg_target *target) > return rc; > } > > - while (tries) { > - /* Open first raw device */ > - fsi->fd = open(path, O_RDWR | O_SYNC); > - if (fsi->fd >= 0) { > - free(path); > - return 0; > - } > - tries--; > - > - /* Scan */ > - kernel_fsi_scan_devices(); > - sleep(1); > - } > - if (fsi->fd < 0) { > - PR_ERROR("Unable to open %s\n", path); > + /* Always scan the fsi bus once */ > + kernel_fsi_scan_devices(); As your comment says, this changes the behavior to always scan even if the sysfs hierarchy is present. Previously it would try to open the "raw" file on the first master and if it was present it wouldn't disturb the setup. Is there any reason you need to force a scan? > + sleep(1); > + > + /* Open first raw device */ > + fsi->fd = open(path, O_RDWR | O_SYNC); > + if (fsi->fd >= 0) { > free(path); > + return 0; > } > > + PR_ERROR("Unable to open %s\n", path); > + free(path); > return -1; > } > > -- > 2.26.2 > > -- > Pdbg mailing list > Pdbg@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/pdbg
On Tue, 2020-06-09 at 07:28 +0000, Joel Stanley wrote: > On Tue, 26 May 2020 at 05:13, Amitay Isaacs <amitay@ozlabs.org> > wrote: > > On rescan, kernel driver re-creates all the slave devices. This > > invalidates all the slave deviced opened before the scan. > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > --- > > libpdbg/kernel.c | 37 +++++++++++++++++++++---------------- > > 1 file changed, 21 insertions(+), 16 deletions(-) > > > > diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c > > index 2cc81f2..912895a 100644 > > --- a/libpdbg/kernel.c > > +++ b/libpdbg/kernel.c > > @@ -116,6 +116,15 @@ static void kernel_fsi_scan_devices(void) > > const char *kernel_path = kernel_get_fsi_path(); > > char *path; > > int rc, fd; > > + static bool scanned = false; > > + > > + /* > > + * On fsi bus rescan, kernel re-creates all the slave > > device entries. > > + * It means any currently open devices will be invalid and > > need to be > > + * re-opened. So avoid scanning multiple times. > > + */ > > + if (scanned) > > + return; > > > > if (!kernel_path) > > return; > > @@ -139,12 +148,13 @@ static void kernel_fsi_scan_devices(void) > > > > free(path); > > close(fd); > > + > > + scanned = true; > > } > > > > int kernel_fsi_probe(struct pdbg_target *target) > > { > > struct fsi *fsi = target_to_fsi(target); > > - int tries = 5; > > int rc; > > const char *kernel_path = kernel_get_fsi_path(); > > const char *fsi_path; > > @@ -162,24 +172,19 @@ int kernel_fsi_probe(struct pdbg_target > > *target) > > return rc; > > } > > > > - while (tries) { > > - /* Open first raw device */ > > - fsi->fd = open(path, O_RDWR | O_SYNC); > > - if (fsi->fd >= 0) { > > - free(path); > > - return 0; > > - } > > - tries--; > > - > > - /* Scan */ > > - kernel_fsi_scan_devices(); > > - sleep(1); > > - } > > - if (fsi->fd < 0) { > > - PR_ERROR("Unable to open %s\n", path); > > + /* Always scan the fsi bus once */ > > + kernel_fsi_scan_devices(); > > As your comment says, this changes the behavior to always scan even > if > the sysfs hierarchy is present. Previously it would try to open the > "raw" file on the first master and if it was present it wouldn't > disturb the setup. In the earlier case, we tried to open only one device, so rescanning didn't matter. But if device 1 is opened and if device 2 cannot be opened, then we cannot scan, as it makes the already open fd invalid. So the only way was to either scan once or no scan at all. Doing a scan is probably harmful on a working system, as other software might have open fd for the raw device. But on a broken system, you would want to do a scan. I am also fine with completely dropping the scanning of fsi bus from pdbg. > Is there any reason you need to force a scan? > > > + sleep(1); > > + > > + /* Open first raw device */ > > + fsi->fd = open(path, O_RDWR | O_SYNC); > > + if (fsi->fd >= 0) { > > free(path); > > + return 0; > > } > > > > + PR_ERROR("Unable to open %s\n", path); > > + free(path); > > return -1; > > } > > > > -- > > 2.26.2 > > > > -- > > Pdbg mailing list > > Pdbg@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/pdbg Amitay.
On Tue, 9 Jun 2020 at 14:16, Amitay Isaacs <amitay@ozlabs.org> wrote: > > On Tue, 2020-06-09 at 07:28 +0000, Joel Stanley wrote: > > On Tue, 26 May 2020 at 05:13, Amitay Isaacs <amitay@ozlabs.org> > > wrote: > > > On rescan, kernel driver re-creates all the slave devices. This > > > invalidates all the slave deviced opened before the scan. > > > - if (fsi->fd < 0) { > > > - PR_ERROR("Unable to open %s\n", path); > > > + /* Always scan the fsi bus once */ > > > + kernel_fsi_scan_devices(); > > > > As your comment says, this changes the behavior to always scan even > > if > > the sysfs hierarchy is present. Previously it would try to open the > > "raw" file on the first master and if it was present it wouldn't > > disturb the setup. > > In the earlier case, we tried to open only one device, so rescanning > didn't matter. But if device 1 is opened and if device 2 cannot be > opened, then we cannot scan, as it makes the already open fd invalid. > So the only way was to either scan once or no scan at all. > > Doing a scan is probably harmful on a working system, as other software > might have open fd for the raw device. But on a broken system, you > would want to do a scan. I am also fine with completely dropping the > scanning of fsi bus from pdbg. Thanks for the explanation. I think we will need to drop the scan (or introduce a --scan option), or else pdbg becomes far less useful in inspecting a live system. I would prefer a --scan option so users don't need to dig around in sysfs.
On Wed, 2020-06-10 at 03:15 +0000, Joel Stanley wrote: > On Tue, 9 Jun 2020 at 14:16, Amitay Isaacs <amitay@ozlabs.org> wrote: > > On Tue, 2020-06-09 at 07:28 +0000, Joel Stanley wrote: > > > On Tue, 26 May 2020 at 05:13, Amitay Isaacs <amitay@ozlabs.org> > > > wrote: > > > > On rescan, kernel driver re-creates all the slave > > > > devices. This > > > > invalidates all the slave deviced opened before the scan. > > > > - if (fsi->fd < 0) { > > > > - PR_ERROR("Unable to open %s\n", path); > > > > + /* Always scan the fsi bus once */ > > > > + kernel_fsi_scan_devices(); > > > > > > As your comment says, this changes the behavior to always scan > > > even > > > if > > > the sysfs hierarchy is present. Previously it would try to open > > > the > > > "raw" file on the first master and if it was present it wouldn't > > > disturb the setup. > > > > In the earlier case, we tried to open only one device, so > > rescanning > > didn't matter. But if device 1 is opened and if device 2 cannot be > > opened, then we cannot scan, as it makes the already open fd > > invalid. > > So the only way was to either scan once or no scan at all. > > > > Doing a scan is probably harmful on a working system, as other > > software > > might have open fd for the raw device. But on a broken system, you > > would want to do a scan. I am also fine with completely dropping > > the > > scanning of fsi bus from pdbg. > > Thanks for the explanation. > > I think we will need to drop the scan (or introduce a --scan option), > or else pdbg becomes far less useful in inspecting a live system. > > I would prefer a --scan option so users don't need to dig around in > sysfs. Turns out it's not that difficult to scan only on the first probe. This avoids the need for --scan option, which would be more difficult to plumb through libpdbg. v2 patch set coming. Amitay.
diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c index 2cc81f2..912895a 100644 --- a/libpdbg/kernel.c +++ b/libpdbg/kernel.c @@ -116,6 +116,15 @@ static void kernel_fsi_scan_devices(void) const char *kernel_path = kernel_get_fsi_path(); char *path; int rc, fd; + static bool scanned = false; + + /* + * On fsi bus rescan, kernel re-creates all the slave device entries. + * It means any currently open devices will be invalid and need to be + * re-opened. So avoid scanning multiple times. + */ + if (scanned) + return; if (!kernel_path) return; @@ -139,12 +148,13 @@ static void kernel_fsi_scan_devices(void) free(path); close(fd); + + scanned = true; } int kernel_fsi_probe(struct pdbg_target *target) { struct fsi *fsi = target_to_fsi(target); - int tries = 5; int rc; const char *kernel_path = kernel_get_fsi_path(); const char *fsi_path; @@ -162,24 +172,19 @@ int kernel_fsi_probe(struct pdbg_target *target) return rc; } - while (tries) { - /* Open first raw device */ - fsi->fd = open(path, O_RDWR | O_SYNC); - if (fsi->fd >= 0) { - free(path); - return 0; - } - tries--; - - /* Scan */ - kernel_fsi_scan_devices(); - sleep(1); - } - if (fsi->fd < 0) { - PR_ERROR("Unable to open %s\n", path); + /* Always scan the fsi bus once */ + kernel_fsi_scan_devices(); + sleep(1); + + /* Open first raw device */ + fsi->fd = open(path, O_RDWR | O_SYNC); + if (fsi->fd >= 0) { free(path); + return 0; } + PR_ERROR("Unable to open %s\n", path); + free(path); return -1; }
On rescan, kernel driver re-creates all the slave devices. This invalidates all the slave deviced opened before the scan. Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- libpdbg/kernel.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-)