| Message ID | 20191023210657.32440-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: update ASoC interfaces | expand |
On 23-10-19, 16:06, Pierre-Louis Bossart wrote: > Changes to the sdw_slave structure needed to solve race conditions on > driver probe. Can you please explain the race you have observed, it would be a very useful to document it as well > > The functionality is added in the next patch. which one..? > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > include/linux/soundwire/sdw.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index 688b40e65c89..a381a596212b 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -545,6 +545,10 @@ struct sdw_slave_ops { > * @node: node for bus list > * @port_ready: Port ready completion flag for each Slave port > * @dev_num: Device Number assigned by Bus > + * @probed: boolean tracking driver state > + * @probe_complete: completion utility to control potential races > + * on startup between driver probe/initialization and SoundWire > + * Slave state changes/imp-def interrupts > */ > struct sdw_slave { > struct sdw_slave_id id; > @@ -559,6 +563,8 @@ struct sdw_slave { > struct list_head node; > struct completion *port_ready; > u16 dev_num; > + bool probed; > + struct completion probe_complete; > }; > > #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev) > -- > 2.20.1
On 11/2/19 11:56 PM, Vinod Koul wrote: > On 23-10-19, 16:06, Pierre-Louis Bossart wrote: >> Changes to the sdw_slave structure needed to solve race conditions on >> driver probe. > > Can you please explain the race you have observed, it would be a very > useful to document it as well the races are explained in the [PATCH 00/18] soundwire: code hardening and suspend-resume support series. >> >> The functionality is added in the next patch. > > which one..? [PATCH 00/18] soundwire: code hardening and suspend-resume support > >> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> include/linux/soundwire/sdw.h | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h >> index 688b40e65c89..a381a596212b 100644 >> --- a/include/linux/soundwire/sdw.h >> +++ b/include/linux/soundwire/sdw.h >> @@ -545,6 +545,10 @@ struct sdw_slave_ops { >> * @node: node for bus list >> * @port_ready: Port ready completion flag for each Slave port >> * @dev_num: Device Number assigned by Bus >> + * @probed: boolean tracking driver state >> + * @probe_complete: completion utility to control potential races >> + * on startup between driver probe/initialization and SoundWire >> + * Slave state changes/imp-def interrupts >> */ >> struct sdw_slave { >> struct sdw_slave_id id; >> @@ -559,6 +563,8 @@ struct sdw_slave { >> struct list_head node; >> struct completion *port_ready; >> u16 dev_num; >> + bool probed; >> + struct completion probe_complete; >> }; >> >> #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev) >> -- >> 2.20.1 >
On 04-11-19, 08:32, Pierre-Louis Bossart wrote: > > > On 11/2/19 11:56 PM, Vinod Koul wrote: > > On 23-10-19, 16:06, Pierre-Louis Bossart wrote: > > > Changes to the sdw_slave structure needed to solve race conditions on > > > driver probe. > > > > Can you please explain the race you have observed, it would be a very > > useful to document it as well > > the races are explained in the [PATCH 00/18] soundwire: code hardening and > suspend-resume support series. It would make sense to explain it here as well to give details to reviewers, there is nothing wrong with too much detail! > > > > > > The functionality is added in the next patch. > > > > which one..? > > [PATCH 00/18] soundwire: code hardening and suspend-resume support Yeah great! let me play detective with 18 patch series. I asked for a patch and got a series! Again, please help the maintainer to help you. We would love to see this merged as well, but please step up and give more details in cover letter and changelogs. I shouldn't need to do guesswork and scan through the inbox to find the context!
On 11/7/19 10:29 PM, Vinod Koul wrote: > On 04-11-19, 08:32, Pierre-Louis Bossart wrote: >> >> >> On 11/2/19 11:56 PM, Vinod Koul wrote: >>> On 23-10-19, 16:06, Pierre-Louis Bossart wrote: >>>> Changes to the sdw_slave structure needed to solve race conditions on >>>> driver probe. >>> >>> Can you please explain the race you have observed, it would be a very >>> useful to document it as well >> >> the races are explained in the [PATCH 00/18] soundwire: code hardening and >> suspend-resume support series. > > It would make sense to explain it here as well to give details to > reviewers, there is nothing wrong with too much detail! > >>>> >>>> The functionality is added in the next patch. >>> >>> which one..? >> >> [PATCH 00/18] soundwire: code hardening and suspend-resume support > > Yeah great! let me play detective with 18 patch series. I asked for a > patch and got a series! > > Again, please help the maintainer to help you. We would love to see this > merged as well, but please step up and give more details in cover > letter and changelogs. I shouldn't need to do guesswork and scan through the > inbox to find the context! We are clearly not going anywhere. I partitioned the patches to make your maintainer life easier and help the integration of SoundWire across two trees. All I get is negative feedback, grand-standing, and zero comments on actual changes. For the record, I am mindful of reviewer/maintainer workload, and I did contact you in September to check your availability and provided a pointer to initial code changes. I did send a first version a week prior to your travel/vacation, I resend another version when you were back and waited yet another two weeks to resend a second version. I also contacted Takashi, Mark and you to suggest this code partition, and did not get any pushback. It's not like I am pushing stuff down your throat, I have been patient and considerate. Please start with the patches "soundwire: code hardening and suspend-resume support" and come back to this interface description when you have reviewed these changes. It's not detective work, it's working around the consequences of having separate trees for Audio and SoundWire.
On Fri, 2019-11-08 at 08:55 -0600, Pierre-Louis Bossart wrote: > Please start with the patches "soundwire: code hardening and > suspend-resume support" and come back to this interface description when > you have reviewed these changes. It's not detective work, it's working > around the consequences of having separate trees for Audio and SoundWire. Separate trees seem to be clearly not working well for everyone atm. I'm reading the discomfort on all sides here. Vinod, how often do you merge on ALSA/ASoC ? Would it not make more sense to be part of ALSA to ease workflow (including yours) ? This model worked well for soundwire's predecessors (AC97 and HDA) which like soundwire were primarily audio busses with secondary non audio features. Liam
On 08-11-19, 08:55, Pierre-Louis Bossart wrote: > > > On 11/7/19 10:29 PM, Vinod Koul wrote: > > On 04-11-19, 08:32, Pierre-Louis Bossart wrote: > > > > > > > > > On 11/2/19 11:56 PM, Vinod Koul wrote: > > > > On 23-10-19, 16:06, Pierre-Louis Bossart wrote: > > > > > Changes to the sdw_slave structure needed to solve race conditions on > > > > > driver probe. > > > > > > > > Can you please explain the race you have observed, it would be a very > > > > useful to document it as well > > > > > > the races are explained in the [PATCH 00/18] soundwire: code hardening and > > > suspend-resume support series. > > > > It would make sense to explain it here as well to give details to > > reviewers, there is nothing wrong with too much detail! > > > > > > > > > > > > The functionality is added in the next patch. > > > > > > > > which one..? > > > > > > [PATCH 00/18] soundwire: code hardening and suspend-resume support > > > > Yeah great! let me play detective with 18 patch series. I asked for a > > patch and got a series! > > > > Again, please help the maintainer to help you. We would love to see this > > merged as well, but please step up and give more details in cover > > letter and changelogs. I shouldn't need to do guesswork and scan through the > > inbox to find the context! > > We are clearly not going anywhere. Correct as you don't seem to provide clear answers, I am asking again which patch implements the new fields added here, how difficult is it to provide the *specific* patch which implements this so that I can compare the implementation and see why this is needed and apply if fine! But no you will not provide a clear answer and start ranting! > I partitioned the patches to make your maintainer life easier and help the > integration of SoundWire across two trees. All I get is negative feedback, > grand-standing, and zero comments on actual changes. No you get asked specific question which you do not like and start off on a tangent! > For the record, I am mindful of reviewer/maintainer workload, and I did > contact you in September to check your availability and provided a pointer > to initial code changes. I did send a first version a week prior to your > travel/vacation, I resend another version when you were back and waited yet > another two weeks to resend a second version. I also contacted Takashi, Mark > and you to suggest this code partition, and did not get any pushback. It's > not like I am pushing stuff down your throat, I have been patient and > considerate. > > Please start with the patches "soundwire: code hardening and suspend-resume > support" and come back to this interface description when you have reviewed > these changes. It's not detective work, it's working around the consequences > of having separate trees for Audio and SoundWire. Again, which patch in the series does implement these new members!
On 11/9/19 5:12 AM, Vinod Koul wrote: > On 08-11-19, 08:55, Pierre-Louis Bossart wrote: >> >> >> On 11/7/19 10:29 PM, Vinod Koul wrote: >>> On 04-11-19, 08:32, Pierre-Louis Bossart wrote: >>>> >>>> >>>> On 11/2/19 11:56 PM, Vinod Koul wrote: >>>>> On 23-10-19, 16:06, Pierre-Louis Bossart wrote: >>>>>> Changes to the sdw_slave structure needed to solve race conditions on >>>>>> driver probe. >>>>> >>>>> Can you please explain the race you have observed, it would be a very >>>>> useful to document it as well >>>> >>>> the races are explained in the [PATCH 00/18] soundwire: code hardening and >>>> suspend-resume support series. >>> >>> It would make sense to explain it here as well to give details to >>> reviewers, there is nothing wrong with too much detail! >>> >>>>>> >>>>>> The functionality is added in the next patch. >>>>> >>>>> which one..? >>>> >>>> [PATCH 00/18] soundwire: code hardening and suspend-resume support >>> >>> Yeah great! let me play detective with 18 patch series. I asked for a >>> patch and got a series! >>> >>> Again, please help the maintainer to help you. We would love to see this >>> merged as well, but please step up and give more details in cover >>> letter and changelogs. I shouldn't need to do guesswork and scan through the >>> inbox to find the context! >> >> We are clearly not going anywhere. > > Correct as you don't seem to provide clear answers, I am asking again > which patch implements the new fields added here, how difficult is it to > provide the *specific* patch which implements this so that I can compare > the implementation and see why this is needed and apply if fine! > > But no you will not provide a clear answer and start ranting! > >> I partitioned the patches to make your maintainer life easier and help the >> integration of SoundWire across two trees. All I get is negative feedback, >> grand-standing, and zero comments on actual changes. > > No you get asked specific question which you do not like and start off > on a tangent! > >> For the record, I am mindful of reviewer/maintainer workload, and I did >> contact you in September to check your availability and provided a pointer >> to initial code changes. I did send a first version a week prior to your >> travel/vacation, I resend another version when you were back and waited yet >> another two weeks to resend a second version. I also contacted Takashi, Mark >> and you to suggest this code partition, and did not get any pushback. It's >> not like I am pushing stuff down your throat, I have been patient and >> considerate. >> >> Please start with the patches "soundwire: code hardening and suspend-resume >> support" and come back to this interface description when you have reviewed >> these changes. It's not detective work, it's working around the consequences >> of having separate trees for Audio and SoundWire. > > Again, which patch in the series does implement these new members! It's really straightforward...here is the match between headers and functionality: [PATCH v2 1/5] soundwire: sdw_slave: add new fields to track probe status [PATCH v2 02/19] soundwire: fix race between driver probe and update_status callback [PATCH v2 2/5] soundwire: add enumeration_complete structure [PATCH v2 12/19] soundwire: add enumeration_complete signaling [PATCH v2 3/5] soundwire: add initialization_complete definition [PATCH v2 13/19] soundwire: bus: add initialization_complete signaling [PATCH v2 4/5] soundwire: intel: update interfaces between ASoC and SoundWire [PATCH v2 5/5] soundwire: intel: update stream callbacks for hwparams/free stream operations [PATCH v2 13/14] soundwire: intel: free all resources on hw_free() I suggested an approach that you didn't comment on, and now I am not sure what to make of the heated wording and exclamation marks. You did not answer to Liam's question on links between ASoC/SoundWire - despite the fact that drivers/soundwire cannot be an isolated subsystem, both the Intel and Qualcomm solutions have a big fat 'depends on SND_SOC'. At this point I am formally asking for your view and guidance on how we are going to do the SoundWire/ASoC integration. It's now your time to make suggestions on what the flow should be between you and Mark/Takashi. If you don't want this initial change to the header files, then what is your proposal?
On 11-11-19, 10:34, Pierre-Louis Bossart wrote: > > > On 11/9/19 5:12 AM, Vinod Koul wrote: > > On 08-11-19, 08:55, Pierre-Louis Bossart wrote: > > > > > > > > > On 11/7/19 10:29 PM, Vinod Koul wrote: > > > > On 04-11-19, 08:32, Pierre-Louis Bossart wrote: > > > > > > > > > > > > > > > On 11/2/19 11:56 PM, Vinod Koul wrote: > > > > > > On 23-10-19, 16:06, Pierre-Louis Bossart wrote: > > > > > > > Changes to the sdw_slave structure needed to solve race conditions on > > > > > > > driver probe. > > > > > > > > > > > > Can you please explain the race you have observed, it would be a very > > > > > > useful to document it as well > > > > > > > > > > the races are explained in the [PATCH 00/18] soundwire: code hardening and > > > > > suspend-resume support series. > > > > > > > > It would make sense to explain it here as well to give details to > > > > reviewers, there is nothing wrong with too much detail! > > > > > > > > > > > > > > > > > > The functionality is added in the next patch. > > > > > > > > > > > > which one..? > > > > > > > > > > [PATCH 00/18] soundwire: code hardening and suspend-resume support > > > > > > > > Yeah great! let me play detective with 18 patch series. I asked for a > > > > patch and got a series! > > > > > > > > Again, please help the maintainer to help you. We would love to see this > > > > merged as well, but please step up and give more details in cover > > > > letter and changelogs. I shouldn't need to do guesswork and scan through the > > > > inbox to find the context! > > > > > > We are clearly not going anywhere. > > > > Correct as you don't seem to provide clear answers, I am asking again > > which patch implements the new fields added here, how difficult is it to > > provide the *specific* patch which implements this so that I can compare > > the implementation and see why this is needed and apply if fine! > > > > But no you will not provide a clear answer and start ranting! > > > > > I partitioned the patches to make your maintainer life easier and help the > > > integration of SoundWire across two trees. All I get is negative feedback, > > > grand-standing, and zero comments on actual changes. > > > > No you get asked specific question which you do not like and start off > > on a tangent! > > > > > For the record, I am mindful of reviewer/maintainer workload, and I did > > > contact you in September to check your availability and provided a pointer > > > to initial code changes. I did send a first version a week prior to your > > > travel/vacation, I resend another version when you were back and waited yet > > > another two weeks to resend a second version. I also contacted Takashi, Mark > > > and you to suggest this code partition, and did not get any pushback. It's > > > not like I am pushing stuff down your throat, I have been patient and > > > considerate. > > > > > > Please start with the patches "soundwire: code hardening and suspend-resume > > > support" and come back to this interface description when you have reviewed > > > these changes. It's not detective work, it's working around the consequences > > > of having separate trees for Audio and SoundWire. > > > > Again, which patch in the series does implement these new members! > > It's really straightforward...here is the match between headers and > functionality: > > [PATCH v2 1/5] soundwire: sdw_slave: add new fields to track probe status > [PATCH v2 02/19] soundwire: fix race between driver probe and update_status > callback > > [PATCH v2 2/5] soundwire: add enumeration_complete structure > [PATCH v2 12/19] soundwire: add enumeration_complete signaling > > [PATCH v2 3/5] soundwire: add initialization_complete definition > [PATCH v2 13/19] soundwire: bus: add initialization_complete signaling > > [PATCH v2 4/5] soundwire: intel: update interfaces between ASoC and > SoundWire > [PATCH v2 5/5] soundwire: intel: update stream callbacks for hwparams/free > stream operations > [PATCH v2 13/14] soundwire: intel: free all resources on hw_free() Thanks for the pointers, I will look at these patches and do the needful for this series > I suggested an approach that you didn't comment on, and now I am not sure > what to make of the heated wording and exclamation marks. You did not answer > to Liam's question on links between ASoC/SoundWire - despite the fact that > drivers/soundwire cannot be an isolated subsystem, both the Intel and > Qualcomm solutions have a big fat 'depends on SND_SOC'. > > At this point I am formally asking for your view and guidance on how we are > going to do the SoundWire/ASoC integration. It's now your time to make > suggestions on what the flow should be between you and Mark/Takashi. If you > don't want this initial change to the header files, then what is your > proposal? It is going to be as it would be for any other subsystem. Please mention in the cover letter about required dependency. In case asoc needs this I will create a immutable tag and in reverse case Mark will do so. It is not really an issue if we get the information ahead of time
>> At this point I am formally asking for your view and guidance on how we are >> going to do the SoundWire/ASoC integration. It's now your time to make >> suggestions on what the flow should be between you and Mark/Takashi. If you >> don't want this initial change to the header files, then what is your >> proposal? > > It is going to be as it would be for any other subsystem. Please mention > in the cover letter about required dependency. In case asoc needs this I > will create a immutable tag and in reverse case Mark will do so. > > It is not really an issue if we get the information ahead of time I added a whole set of comments on the race conditions and ran them by people with limited knowledge of SoundWire to see if the explanations made sense and why those header files were changed. Will send this later today as a v3.
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 688b40e65c89..a381a596212b 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -545,6 +545,10 @@ struct sdw_slave_ops { * @node: node for bus list * @port_ready: Port ready completion flag for each Slave port * @dev_num: Device Number assigned by Bus + * @probed: boolean tracking driver state + * @probe_complete: completion utility to control potential races + * on startup between driver probe/initialization and SoundWire + * Slave state changes/imp-def interrupts */ struct sdw_slave { struct sdw_slave_id id; @@ -559,6 +563,8 @@ struct sdw_slave { struct list_head node; struct completion *port_ready; u16 dev_num; + bool probed; + struct completion probe_complete; }; #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
Changes to the sdw_slave structure needed to solve race conditions on driver probe. The functionality is added in the next patch. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- include/linux/soundwire/sdw.h | 6 ++++++ 1 file changed, 6 insertions(+)