Hi Kaihenfeng,
Thanks for your patch suggestion! I'm semantically not sure it is the right thing - to clarify your theory is that before it checked !resuming and before had the check for !cdev maybe just to avoid a deference error. And now you assume that instead of !cdev it should check if there is a cdev there.
I'm unsure - if !cdev was indeed just to protect the dereference then maybe no check at all might be better. Which would then read "if the event is IO_SCH_ORPH_UNREG or IO_SCH_UNREG then do css_sch_device_unregister.
But that I'm not immediately convinced doesn't mean much and it is easy to test and surely worth a try, so I ran v5.11 (bad) plus your patch and the result will be useful to know in any case. It is working fine, that much I can tell you.
But if my thought above was right (it was only there to avoid the potential deference error), then why check it at all. If the condition cdev==NULL is possible it would now skip to to fully remove it - we might not need that at all.
And Since I brought up the idea of dropping the cdev check entirely that was worth a try as well. So now the third check of this morning is for:
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -1525,8 +1525,7 @@ static int io_subchannel_sch_event(struct subchannel *sch, int process)
switch (action) {
case IO_SCH_ORPH_UNREG:
case IO_SCH_UNREG:
- if (!cdev)
- css_sch_device_unregister(sch);
+ css_sch_device_unregister(sch); break;
case IO_SCH_ORPH_ATTACH:
case IO_SCH_UNREG_ATTACH:
My patch with that change - in my test - is working as well.
Neither of the solutions has triggered other regressions in my setup - but then there are so many potential use-cases that I can't be sure without a further revew by subject matter experts.
So a summary of the recent tests:
5.11.0-16-generic #17+lp1925211v202104201520 (Seths full revert) - working
5.11.0lp1925211-patch-kaihengfeng-dirty - working
5.11.0nocdevcheck-paelzer-dirty - working
I think we'd want an answer from the IBM devs which solution (full revert, kaihenfeng patch, cpaelzer patch, another approach) they would prefer - then we can submit it upstream for them to include officially and we can carry it as delta until we rebase onto a version that has it applied anyway.
Hi Kaihenfeng, device_ unregister.
Thanks for your patch suggestion! I'm semantically not sure it is the right thing - to clarify your theory is that before it checked !resuming and before had the check for !cdev maybe just to avoid a deference error. And now you assume that instead of !cdev it should check if there is a cdev there.
I'm unsure - if !cdev was indeed just to protect the dereference then maybe no check at all might be better. Which would then read "if the event is IO_SCH_ORPH_UNREG or IO_SCH_UNREG then do css_sch_
But that I'm not immediately convinced doesn't mean much and it is easy to test and surely worth a try, so I ran v5.11 (bad) plus your patch and the result will be useful to know in any case. It is working fine, that much I can tell you.
But if my thought above was right (it was only there to avoid the potential deference error), then why check it at all. If the condition cdev==NULL is possible it would now skip to to fully remove it - we might not need that at all. s390/cio/ device. c s390/cio/ device. c sch_event( struct subchannel *sch, int process) device_ unregister( sch); device_ unregister( sch);
break; UNREG_ATTACH:
And Since I brought up the idea of dropping the cdev check entirely that was worth a try as well. So now the third check of this morning is for:
--- a/drivers/
+++ b/drivers/
@@ -1525,8 +1525,7 @@ static int io_subchannel_
switch (action) {
case IO_SCH_ORPH_UNREG:
case IO_SCH_UNREG:
- if (!cdev)
- css_sch_
+ css_sch_
case IO_SCH_ORPH_ATTACH:
case IO_SCH_
My patch with that change - in my test - is working as well.
Neither of the solutions has triggered other regressions in my setup - but then there are so many potential use-cases that I can't be sure without a further revew by subject matter experts.
So a summary of the recent tests:
5.11.0-16-generic #17+lp1925211v2 02104201520 (Seths full revert) - working -patch- kaihengfeng- dirty - working ck-paelzer- dirty - working
5.11.0lp1925211
5.11.0nocdevche
I think we'd want an answer from the IBM devs which solution (full revert, kaihenfeng patch, cpaelzer patch, another approach) they would prefer - then we can submit it upstream for them to include officially and we can carry it as delta until we rebase onto a version that has it applied anyway.
[1]: https:/ /git.kernel. org/pub/ scm/linux/ kernel/ git/torvalds/ linux.git/ commit/ ?id=8cc0dcfdc1c 0e0be107d0288f9 c0cf1f4201be62