| Message ID | 20230426205713.512695-4-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce xe_devcoredump. | expand |
On Wed, Apr 26, 2023 at 04:57:02PM -0400, Rodrigo Vivi wrote: > Unfortunately devcoredump infrastructure does not provide and > interface for us to force the device removal upon the pci_remove > time of our device. > > The devcoredump is linked at the device level, so when in use > it will prevent the module removal, but it doesn't prevent the > call of the pci_remove callback. This callback cannot fail > anyway and we end up clearing and freeing the entire pci device. > > Hence, after we removed the pci device, we shouldn't allow any > read or free operations to avoid segmentation fault. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/xe/xe_devcoredump.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c > index d9531183f03a..a08929c01b75 100644 > --- a/drivers/gpu/drm/xe/xe_devcoredump.c > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c > @@ -42,6 +42,11 @@ > * hang capture. > */ > > +static struct xe_device *coredump_to_xe(const struct xe_devcoredump *coredump) > +{ > + return container_of(coredump, struct xe_device, devcoredump); Confused how still would ever return NULL, can you explain? Matt > +} > + > static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > size_t count, void *data, size_t datalen) > { > @@ -51,6 +56,10 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > struct drm_print_iterator iter; > struct timespec64 ts; > > + /* Our device is gone already... */ > + if (!data || !coredump_to_xe(coredump)) > + return -ENODEV; > + > iter.data = buffer; > iter.offset = 0; > iter.start = offset; > @@ -80,12 +89,16 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > static void xe_devcoredump_free(void *data) > { > struct xe_devcoredump *coredump = data; > - struct xe_device *xe = container_of(coredump, struct xe_device, > - devcoredump); > + > + /* Our device is gone. Nothing to do... */ > + if (!data || !coredump_to_xe(coredump)) > + return; > + > mutex_lock(&coredump->lock); > > coredump->faulty_engine = NULL; > - drm_info(&xe->drm, "Xe device coredump has been deleted.\n"); > + drm_info(&coredump_to_xe(coredump)->drm, > + "Xe device coredump has been deleted.\n"); > > mutex_unlock(&coredump->lock); > } > -- > 2.39.2 >
On Tue, May 02, 2023 at 03:40:50PM +0000, Matthew Brost wrote: > On Wed, Apr 26, 2023 at 04:57:02PM -0400, Rodrigo Vivi wrote: > > Unfortunately devcoredump infrastructure does not provide and > > interface for us to force the device removal upon the pci_remove > > time of our device. > > > > The devcoredump is linked at the device level, so when in use > > it will prevent the module removal, but it doesn't prevent the > > call of the pci_remove callback. This callback cannot fail > > anyway and we end up clearing and freeing the entire pci device. > > > > Hence, after we removed the pci device, we shouldn't allow any > > read or free operations to avoid segmentation fault. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/xe/xe_devcoredump.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c > > index d9531183f03a..a08929c01b75 100644 > > --- a/drivers/gpu/drm/xe/xe_devcoredump.c > > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c > > @@ -42,6 +42,11 @@ > > * hang capture. > > */ > > > > +static struct xe_device *coredump_to_xe(const struct xe_devcoredump *coredump) > > +{ > > + return container_of(coredump, struct xe_device, devcoredump); > > Confused how still would ever return NULL, can you explain? Very good question! I'm honestly still confused myself. There's something not quite right with the device relationship that is getting created with the failling_device and the virtual coredump device. Once failing_device is removed, the devcoredump should be removed as well, or both removals blocked. However this is not what happens. On rmmod xe, the device removal is called and we free all xe structs. The pci device removal is a void function. We cannot fail. The module removal ends up blocked because the relationship, but that doesn't saves the day since the device itself is already gone, by the pci removal function. But the devcoredump device is there and active. There's no callback on devcoredump infra that we could call to force the device removal. Then any read function will hit a NULL xe device and BOOM! This is one of the things I planned to tackle on the devcoredump side after we get this basic infra in use in our driver. This patch allows us to be protected from this scenario while we don't fix this at the devcoredump side. It's worth saying that the devcoredump virtual device is auto removed after a time elapsed... couple minutes? (I can't remember by heart now). > > Matt > > > +} > > + > > static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > > size_t count, void *data, size_t datalen) > > { > > @@ -51,6 +56,10 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > > struct drm_print_iterator iter; > > struct timespec64 ts; > > > > + /* Our device is gone already... */ > > + if (!data || !coredump_to_xe(coredump)) > > + return -ENODEV; > > + > > iter.data = buffer; > > iter.offset = 0; > > iter.start = offset; > > @@ -80,12 +89,16 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > > static void xe_devcoredump_free(void *data) > > { > > struct xe_devcoredump *coredump = data; > > - struct xe_device *xe = container_of(coredump, struct xe_device, > > - devcoredump); > > + > > + /* Our device is gone. Nothing to do... */ > > + if (!data || !coredump_to_xe(coredump)) > > + return; > > + > > mutex_lock(&coredump->lock); > > > > coredump->faulty_engine = NULL; > > - drm_info(&xe->drm, "Xe device coredump has been deleted.\n"); > > + drm_info(&coredump_to_xe(coredump)->drm, > > + "Xe device coredump has been deleted.\n"); > > > > mutex_unlock(&coredump->lock); > > } > > -- > > 2.39.2 > >
On Tue, May 02, 2023 at 01:21:43PM -0400, Rodrigo Vivi wrote: > On Tue, May 02, 2023 at 03:40:50PM +0000, Matthew Brost wrote: > > On Wed, Apr 26, 2023 at 04:57:02PM -0400, Rodrigo Vivi wrote: > > > Unfortunately devcoredump infrastructure does not provide and > > > interface for us to force the device removal upon the pci_remove > > > time of our device. > > > > > > The devcoredump is linked at the device level, so when in use > > > it will prevent the module removal, but it doesn't prevent the > > > call of the pci_remove callback. This callback cannot fail > > > anyway and we end up clearing and freeing the entire pci device. > > > > > > Hence, after we removed the pci device, we shouldn't allow any > > > read or free operations to avoid segmentation fault. > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/xe/xe_devcoredump.c | 19 ++++++++++++++++--- > > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c > > > index d9531183f03a..a08929c01b75 100644 > > > --- a/drivers/gpu/drm/xe/xe_devcoredump.c > > > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c > > > @@ -42,6 +42,11 @@ > > > * hang capture. > > > */ > > > > > > +static struct xe_device *coredump_to_xe(const struct xe_devcoredump *coredump) > > > +{ > > > + return container_of(coredump, struct xe_device, devcoredump); > > > > Confused how still would ever return NULL, can you explain? > > Very good question! I'm honestly still confused myself. > > There's something not quite right with the device relationship that > is getting created with the failling_device and the virtual coredump device. > > Once failing_device is removed, the devcoredump should be removed as well, > or both removals blocked. However this is not what happens. > > On rmmod xe, the device removal is called and we free all xe structs. > The pci device removal is a void function. We cannot fail. The module > removal ends up blocked because the relationship, but that doesn't > saves the day since the device itself is already gone, by the pci > removal function. > > But the devcoredump device is there and active. There's no callback on > devcoredump infra that we could call to force the device removal. Then > any read function will hit a NULL xe device and BOOM! > > This is one of the things I planned to tackle on the devcoredump side > after we get this basic infra in use in our driver. This patch allows > us to be protected from this scenario while we don't fix this at the > devcoredump side. > > It's worth saying that the devcoredump virtual device is auto removed > after a time elapsed... couple minutes? (I can't remember by heart now). > After some serious thought, I think I get this. With that: Reviewed-by: Matthew Brost <matthew.brost@intel.com> But agree this goofy and try to fix this properly after this is merged. > > > > Matt > > > > > +} > > > + > > > static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > > > size_t count, void *data, size_t datalen) > > > { > > > @@ -51,6 +56,10 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > > > struct drm_print_iterator iter; > > > struct timespec64 ts; > > > > > > + /* Our device is gone already... */ > > > + if (!data || !coredump_to_xe(coredump)) > > > + return -ENODEV; > > > + > > > iter.data = buffer; > > > iter.offset = 0; > > > iter.start = offset; > > > @@ -80,12 +89,16 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, > > > static void xe_devcoredump_free(void *data) > > > { > > > struct xe_devcoredump *coredump = data; > > > - struct xe_device *xe = container_of(coredump, struct xe_device, > > > - devcoredump); > > > + > > > + /* Our device is gone. Nothing to do... */ > > > + if (!data || !coredump_to_xe(coredump)) > > > + return; > > > + > > > mutex_lock(&coredump->lock); > > > > > > coredump->faulty_engine = NULL; > > > - drm_info(&xe->drm, "Xe device coredump has been deleted.\n"); > > > + drm_info(&coredump_to_xe(coredump)->drm, > > > + "Xe device coredump has been deleted.\n"); > > > > > > mutex_unlock(&coredump->lock); > > > } > > > -- > > > 2.39.2 > > >
diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c index d9531183f03a..a08929c01b75 100644 --- a/drivers/gpu/drm/xe/xe_devcoredump.c +++ b/drivers/gpu/drm/xe/xe_devcoredump.c @@ -42,6 +42,11 @@ * hang capture. */ +static struct xe_device *coredump_to_xe(const struct xe_devcoredump *coredump) +{ + return container_of(coredump, struct xe_device, devcoredump); +} + static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, size_t count, void *data, size_t datalen) { @@ -51,6 +56,10 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, struct drm_print_iterator iter; struct timespec64 ts; + /* Our device is gone already... */ + if (!data || !coredump_to_xe(coredump)) + return -ENODEV; + iter.data = buffer; iter.offset = 0; iter.start = offset; @@ -80,12 +89,16 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset, static void xe_devcoredump_free(void *data) { struct xe_devcoredump *coredump = data; - struct xe_device *xe = container_of(coredump, struct xe_device, - devcoredump); + + /* Our device is gone. Nothing to do... */ + if (!data || !coredump_to_xe(coredump)) + return; + mutex_lock(&coredump->lock); coredump->faulty_engine = NULL; - drm_info(&xe->drm, "Xe device coredump has been deleted.\n"); + drm_info(&coredump_to_xe(coredump)->drm, + "Xe device coredump has been deleted.\n"); mutex_unlock(&coredump->lock); }
Unfortunately devcoredump infrastructure does not provide and interface for us to force the device removal upon the pci_remove time of our device. The devcoredump is linked at the device level, so when in use it will prevent the module removal, but it doesn't prevent the call of the pci_remove callback. This callback cannot fail anyway and we end up clearing and freeing the entire pci device. Hence, after we removed the pci device, we shouldn't allow any read or free operations to avoid segmentation fault. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/xe/xe_devcoredump.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)