Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On 07/24/2018 09:08 PM, Mathieu Poirier wrote: On Mon, 23 Jul 2018 at 16:27, Suzuki K Poulose wrote: Mathieu, On 07/23/2018 07:22 PM, Mathieu Poirier wrote: On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose wrote: Mathieu, On 19/07/18 21:36, Mathieu Poirier wrote: On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: In coresight perf mode, we need to prepare the sink before starting a session, which is done via set_buffer call back. We then proceed to enable the tracing. If we fail to start the session successfully, we leave the sink configuration unchanged. This was fine for the existing backends as they don't have any state associated with the buffers. But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail. In order to make the operation atomic and to avoid yet another call back, we get rid of the "set_buffer" call back and pass the buffer details via enable() call back to the sink. Suzuki, I'm not sure I understand the problem you're trying to fix there. From the implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the same result been achievable using a callback? We can definitely achieve the results using "set_buffer". But for ETR, we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". But if we failed to enable_path(), we leave the drvdata->perf_data and doesn't clean it up. Now when another session is about to set_buf, we check if perf_data is empty and WARNs otherwise. Because we can't be sure if it belongs to an abandoned session or another active session and we completely messed somewhere in the driver. So, we need a clear_buffer call back if the enable fails, something not really worth. Anyways, there is no point in separating set_buffer and enabling the sink, as the error handling becomes cumbersome as explained above. I'm fine with this patch and supportive of getting rid of callbacks if we can, I just need to understand the exact problem you're after. From looking a your code (and the current implementation), if we succeed in setting the memory for the sink but fail in any of the subsequent steps i.e, enabling the rest of the compoment on the path or the source, the sink is left unchanged. Yes, thats right. And we should WARN (which I missed in this version) if there is a perf_data already for a disabled ETR. Please see my response to the next patch. The changelog for this patch states the following: "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." I did a deep dive in the code and in the current implementation if the source fails to be enabled in etm_event_start() the path and the sink remains unchanged. With your patchset this get fixed because a goto was added to disable the path when such condition occur. As such each component in the path will see its ->disable() callback invoked. In tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in tmc_etr_disable_hw(), so the cleanup on error condition is done properly. As such we wouldn't need a clean_buffer() callback. All of this is right. But we still have a case. e.g, if the ETR is enabled in sysfs mode, coresight_enable_path() will fail after we have set the buffer. And since we don't try to disable the path when we fail at SINK (which is the right thing to do, as we could be potentially disabling the ETR operated in sysfs mode), we leave the perf_data around. And the next session finds a non-empty data. We are in total agreement :o) All I hoping for is to see the sentence "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." removed from the changelog. To me that sounds like you're adding cleanup work in enable() which isn't the case. On the flip side you are making the sink configuration atomic and that is the important part. :-), I will update the change log accordingly. As I said I'm in favour of removing the set_buffer() callback but I wouldn't associated it with ETR state cleanup. If the code can be rearranged in a way that code can be removed then that alone is enough to ju +if (coresight_enable_path(path, CS_MODE_PERF, handle)) Here we already have a handle on "event_data". As such I think this is what we should feed to coresight_enable_path() rather than the handle. That way we don't need to call etm_perf_sink_config(), we just use the data. The advantage of passing on the handle is, we could get all the way upto the "perf_event" for the given session. Passing the event_data will loose that information. i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config | <-event || | The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we handle the information under the event_data. i.e, if we decide to make some changes in the way we store event_data, we need to spill the changes every where. But the
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On 07/24/2018 09:08 PM, Mathieu Poirier wrote: On Mon, 23 Jul 2018 at 16:27, Suzuki K Poulose wrote: Mathieu, On 07/23/2018 07:22 PM, Mathieu Poirier wrote: On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose wrote: Mathieu, On 19/07/18 21:36, Mathieu Poirier wrote: On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: In coresight perf mode, we need to prepare the sink before starting a session, which is done via set_buffer call back. We then proceed to enable the tracing. If we fail to start the session successfully, we leave the sink configuration unchanged. This was fine for the existing backends as they don't have any state associated with the buffers. But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail. In order to make the operation atomic and to avoid yet another call back, we get rid of the "set_buffer" call back and pass the buffer details via enable() call back to the sink. Suzuki, I'm not sure I understand the problem you're trying to fix there. From the implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the same result been achievable using a callback? We can definitely achieve the results using "set_buffer". But for ETR, we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". But if we failed to enable_path(), we leave the drvdata->perf_data and doesn't clean it up. Now when another session is about to set_buf, we check if perf_data is empty and WARNs otherwise. Because we can't be sure if it belongs to an abandoned session or another active session and we completely messed somewhere in the driver. So, we need a clear_buffer call back if the enable fails, something not really worth. Anyways, there is no point in separating set_buffer and enabling the sink, as the error handling becomes cumbersome as explained above. I'm fine with this patch and supportive of getting rid of callbacks if we can, I just need to understand the exact problem you're after. From looking a your code (and the current implementation), if we succeed in setting the memory for the sink but fail in any of the subsequent steps i.e, enabling the rest of the compoment on the path or the source, the sink is left unchanged. Yes, thats right. And we should WARN (which I missed in this version) if there is a perf_data already for a disabled ETR. Please see my response to the next patch. The changelog for this patch states the following: "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." I did a deep dive in the code and in the current implementation if the source fails to be enabled in etm_event_start() the path and the sink remains unchanged. With your patchset this get fixed because a goto was added to disable the path when such condition occur. As such each component in the path will see its ->disable() callback invoked. In tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in tmc_etr_disable_hw(), so the cleanup on error condition is done properly. As such we wouldn't need a clean_buffer() callback. All of this is right. But we still have a case. e.g, if the ETR is enabled in sysfs mode, coresight_enable_path() will fail after we have set the buffer. And since we don't try to disable the path when we fail at SINK (which is the right thing to do, as we could be potentially disabling the ETR operated in sysfs mode), we leave the perf_data around. And the next session finds a non-empty data. We are in total agreement :o) All I hoping for is to see the sentence "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." removed from the changelog. To me that sounds like you're adding cleanup work in enable() which isn't the case. On the flip side you are making the sink configuration atomic and that is the important part. :-), I will update the change log accordingly. As I said I'm in favour of removing the set_buffer() callback but I wouldn't associated it with ETR state cleanup. If the code can be rearranged in a way that code can be removed then that alone is enough to ju +if (coresight_enable_path(path, CS_MODE_PERF, handle)) Here we already have a handle on "event_data". As such I think this is what we should feed to coresight_enable_path() rather than the handle. That way we don't need to call etm_perf_sink_config(), we just use the data. The advantage of passing on the handle is, we could get all the way upto the "perf_event" for the given session. Passing the event_data will loose that information. i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config | <-event || | The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we handle the information under the event_data. i.e, if we decide to make some changes in the way we store event_data, we need to spill the changes every where. But the
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On Mon, 23 Jul 2018 at 16:27, Suzuki K Poulose wrote: > > Mathieu, > > On 07/23/2018 07:22 PM, Mathieu Poirier wrote: > > On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose > > wrote: > >> > >> Mathieu, > >> > >> On 19/07/18 21:36, Mathieu Poirier wrote: > >>> On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: > In coresight perf mode, we need to prepare the sink before > starting a session, which is done via set_buffer call back. > We then proceed to enable the tracing. If we fail to start > the session successfully, we leave the sink configuration > unchanged. This was fine for the existing backends as they > don't have any state associated with the buffers. But with > ETR, we need to keep track of the buffer details and need > to be cleaned up if we fail. In order to make the operation > atomic and to avoid yet another call back, we get rid of > the "set_buffer" call back and pass the buffer details > via enable() call back to the sink. > >>> > >>> Suzuki, > >>> > >>> I'm not sure I understand the problem you're trying to fix there. From > >>> the > >>> implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't > >>> the > >>> same result been achievable using a callback? > >> > >> We can definitely achieve the results using "set_buffer". But for ETR, > >> we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". > >> But if we failed to enable_path(), we leave the drvdata->perf_data > >> and doesn't clean it up. Now when another session is about to set_buf, > >> we check if perf_data is empty and WARNs otherwise. > >> Because we can't be sure if it belongs to an abandoned session or > >> another active session and we completely messed somewhere in the driver. > >> So, we need a clear_buffer call back if the enable fails, something > >> not really worth. Anyways, there is no point in separating set_buffer > >> and enabling the sink, as the error handling becomes cumbersome as > >> explained > >> above. > >> > >>> > >>> I'm fine with this patch and supportive of getting rid of callbacks if we > >>> can, I > >>> just need to understand the exact problem you're after. From looking a > >>> your > >>> code (and the current implementation), if we succeed in setting the > >>> memory for > >>> the sink but fail in any of the subsequent steps i.e, enabling the rest > >>> of the > >>> compoment on the path or the source, the sink is left unchanged. > >> > >> Yes, thats right. And we should WARN (which I missed in this version) if > >> there is a perf_data already for a disabled ETR. Please see my response to > >> the > >> next patch. > > > > The changelog for this patch states the following: "But with ETR, we > > need to keep track of the buffer details and need to be cleaned up if > > we fail." > > > > I did a deep dive in the code and in the current implementation if the > > source fails to be enabled in etm_event_start() the path and the sink > > remains unchanged. With your patchset this get fixed because a goto > > was added to disable the path when such condition occur. As such each > > component in the path will see its ->disable() callback invoked. In > > tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in > > tmc_etr_disable_hw(), so the cleanup on error condition is done > > properly. As such we wouldn't need a clean_buffer() callback. > > All of this is right. But we still have a case. e.g, if the ETR is > enabled in sysfs mode, coresight_enable_path() will fail after we > have set the buffer. And since we don't try to disable the path > when we fail at SINK (which is the right thing to do, as we could > be potentially disabling the ETR operated in sysfs mode), we leave > the perf_data around. And the next session finds a non-empty data. We are in total agreement :o) All I hoping for is to see the sentence "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." removed from the changelog. To me that sounds like you're adding cleanup work in enable() which isn't the case. On the flip side you are making the sink configuration atomic and that is the important part. > > > > > As I said I'm in favour of removing the set_buffer() callback but I > > wouldn't associated it with ETR state cleanup. If the code can be > > rearranged in a way that code can be removed then that alone is enough > > to justify the change. > > > >> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c > b/drivers/hwtracing/coresight/coresight-etm-perf.c > index 3cc4a0b..12a247d 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event > *event, int flags) > path = etm_event_cpu_path(event_data, cpu); > /* We need a sink, no need to continue without one */ >
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On Mon, 23 Jul 2018 at 16:27, Suzuki K Poulose wrote: > > Mathieu, > > On 07/23/2018 07:22 PM, Mathieu Poirier wrote: > > On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose > > wrote: > >> > >> Mathieu, > >> > >> On 19/07/18 21:36, Mathieu Poirier wrote: > >>> On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: > In coresight perf mode, we need to prepare the sink before > starting a session, which is done via set_buffer call back. > We then proceed to enable the tracing. If we fail to start > the session successfully, we leave the sink configuration > unchanged. This was fine for the existing backends as they > don't have any state associated with the buffers. But with > ETR, we need to keep track of the buffer details and need > to be cleaned up if we fail. In order to make the operation > atomic and to avoid yet another call back, we get rid of > the "set_buffer" call back and pass the buffer details > via enable() call back to the sink. > >>> > >>> Suzuki, > >>> > >>> I'm not sure I understand the problem you're trying to fix there. From > >>> the > >>> implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't > >>> the > >>> same result been achievable using a callback? > >> > >> We can definitely achieve the results using "set_buffer". But for ETR, > >> we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". > >> But if we failed to enable_path(), we leave the drvdata->perf_data > >> and doesn't clean it up. Now when another session is about to set_buf, > >> we check if perf_data is empty and WARNs otherwise. > >> Because we can't be sure if it belongs to an abandoned session or > >> another active session and we completely messed somewhere in the driver. > >> So, we need a clear_buffer call back if the enable fails, something > >> not really worth. Anyways, there is no point in separating set_buffer > >> and enabling the sink, as the error handling becomes cumbersome as > >> explained > >> above. > >> > >>> > >>> I'm fine with this patch and supportive of getting rid of callbacks if we > >>> can, I > >>> just need to understand the exact problem you're after. From looking a > >>> your > >>> code (and the current implementation), if we succeed in setting the > >>> memory for > >>> the sink but fail in any of the subsequent steps i.e, enabling the rest > >>> of the > >>> compoment on the path or the source, the sink is left unchanged. > >> > >> Yes, thats right. And we should WARN (which I missed in this version) if > >> there is a perf_data already for a disabled ETR. Please see my response to > >> the > >> next patch. > > > > The changelog for this patch states the following: "But with ETR, we > > need to keep track of the buffer details and need to be cleaned up if > > we fail." > > > > I did a deep dive in the code and in the current implementation if the > > source fails to be enabled in etm_event_start() the path and the sink > > remains unchanged. With your patchset this get fixed because a goto > > was added to disable the path when such condition occur. As such each > > component in the path will see its ->disable() callback invoked. In > > tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in > > tmc_etr_disable_hw(), so the cleanup on error condition is done > > properly. As such we wouldn't need a clean_buffer() callback. > > All of this is right. But we still have a case. e.g, if the ETR is > enabled in sysfs mode, coresight_enable_path() will fail after we > have set the buffer. And since we don't try to disable the path > when we fail at SINK (which is the right thing to do, as we could > be potentially disabling the ETR operated in sysfs mode), we leave > the perf_data around. And the next session finds a non-empty data. We are in total agreement :o) All I hoping for is to see the sentence "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." removed from the changelog. To me that sounds like you're adding cleanup work in enable() which isn't the case. On the flip side you are making the sink configuration atomic and that is the important part. > > > > > As I said I'm in favour of removing the set_buffer() callback but I > > wouldn't associated it with ETR state cleanup. If the code can be > > rearranged in a way that code can be removed then that alone is enough > > to justify the change. > > > >> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c > b/drivers/hwtracing/coresight/coresight-etm-perf.c > index 3cc4a0b..12a247d 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event > *event, int flags) > path = etm_event_cpu_path(event_data, cpu); > /* We need a sink, no need to continue without one */ >
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
Mathieu, On 07/23/2018 07:22 PM, Mathieu Poirier wrote: On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose wrote: Mathieu, On 19/07/18 21:36, Mathieu Poirier wrote: On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: In coresight perf mode, we need to prepare the sink before starting a session, which is done via set_buffer call back. We then proceed to enable the tracing. If we fail to start the session successfully, we leave the sink configuration unchanged. This was fine for the existing backends as they don't have any state associated with the buffers. But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail. In order to make the operation atomic and to avoid yet another call back, we get rid of the "set_buffer" call back and pass the buffer details via enable() call back to the sink. Suzuki, I'm not sure I understand the problem you're trying to fix there. From the implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the same result been achievable using a callback? We can definitely achieve the results using "set_buffer". But for ETR, we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". But if we failed to enable_path(), we leave the drvdata->perf_data and doesn't clean it up. Now when another session is about to set_buf, we check if perf_data is empty and WARNs otherwise. Because we can't be sure if it belongs to an abandoned session or another active session and we completely messed somewhere in the driver. So, we need a clear_buffer call back if the enable fails, something not really worth. Anyways, there is no point in separating set_buffer and enabling the sink, as the error handling becomes cumbersome as explained above. I'm fine with this patch and supportive of getting rid of callbacks if we can, I just need to understand the exact problem you're after. From looking a your code (and the current implementation), if we succeed in setting the memory for the sink but fail in any of the subsequent steps i.e, enabling the rest of the compoment on the path or the source, the sink is left unchanged. Yes, thats right. And we should WARN (which I missed in this version) if there is a perf_data already for a disabled ETR. Please see my response to the next patch. The changelog for this patch states the following: "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." I did a deep dive in the code and in the current implementation if the source fails to be enabled in etm_event_start() the path and the sink remains unchanged. With your patchset this get fixed because a goto was added to disable the path when such condition occur. As such each component in the path will see its ->disable() callback invoked. In tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in tmc_etr_disable_hw(), so the cleanup on error condition is done properly. As such we wouldn't need a clean_buffer() callback. All of this is right. But we still have a case. e.g, if the ETR is enabled in sysfs mode, coresight_enable_path() will fail after we have set the buffer. And since we don't try to disable the path when we fail at SINK (which is the right thing to do, as we could be potentially disabling the ETR operated in sysfs mode), we leave the perf_data around. And the next session finds a non-empty data. As I said I'm in favour of removing the set_buffer() callback but I wouldn't associated it with ETR state cleanup. If the code can be rearranged in a way that code can be removed then that alone is enough to justify the change. diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 3cc4a0b..12a247d 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags) path = etm_event_cpu_path(event_data, cpu); /* We need a sink, no need to continue without one */ sink = coresight_get_sink(path); -if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer)) -goto fail_end_stop; - -/* Configure the sink */ -if (sink_ops(sink)->set_buffer(sink, handle, - event_data->snk_config)) +if (WARN_ON_ONCE(!sink)) goto fail_end_stop; /* Nothing will happen without a path */ -if (coresight_enable_path(path, CS_MODE_PERF)) +if (coresight_enable_path(path, CS_MODE_PERF, handle)) Here we already have a handle on "event_data". As such I think this is what we should feed to coresight_enable_path() rather than the handle. That way we don't need to call etm_perf_sink_config(), we just use the data. The advantage of passing on the handle is, we could get all the way upto the "perf_event" for the given session. Passing the event_data will loose that information. i.e, perf_event->
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
Mathieu, On 07/23/2018 07:22 PM, Mathieu Poirier wrote: On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose wrote: Mathieu, On 19/07/18 21:36, Mathieu Poirier wrote: On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: In coresight perf mode, we need to prepare the sink before starting a session, which is done via set_buffer call back. We then proceed to enable the tracing. If we fail to start the session successfully, we leave the sink configuration unchanged. This was fine for the existing backends as they don't have any state associated with the buffers. But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail. In order to make the operation atomic and to avoid yet another call back, we get rid of the "set_buffer" call back and pass the buffer details via enable() call back to the sink. Suzuki, I'm not sure I understand the problem you're trying to fix there. From the implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the same result been achievable using a callback? We can definitely achieve the results using "set_buffer". But for ETR, we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". But if we failed to enable_path(), we leave the drvdata->perf_data and doesn't clean it up. Now when another session is about to set_buf, we check if perf_data is empty and WARNs otherwise. Because we can't be sure if it belongs to an abandoned session or another active session and we completely messed somewhere in the driver. So, we need a clear_buffer call back if the enable fails, something not really worth. Anyways, there is no point in separating set_buffer and enabling the sink, as the error handling becomes cumbersome as explained above. I'm fine with this patch and supportive of getting rid of callbacks if we can, I just need to understand the exact problem you're after. From looking a your code (and the current implementation), if we succeed in setting the memory for the sink but fail in any of the subsequent steps i.e, enabling the rest of the compoment on the path or the source, the sink is left unchanged. Yes, thats right. And we should WARN (which I missed in this version) if there is a perf_data already for a disabled ETR. Please see my response to the next patch. The changelog for this patch states the following: "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." I did a deep dive in the code and in the current implementation if the source fails to be enabled in etm_event_start() the path and the sink remains unchanged. With your patchset this get fixed because a goto was added to disable the path when such condition occur. As such each component in the path will see its ->disable() callback invoked. In tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in tmc_etr_disable_hw(), so the cleanup on error condition is done properly. As such we wouldn't need a clean_buffer() callback. All of this is right. But we still have a case. e.g, if the ETR is enabled in sysfs mode, coresight_enable_path() will fail after we have set the buffer. And since we don't try to disable the path when we fail at SINK (which is the right thing to do, as we could be potentially disabling the ETR operated in sysfs mode), we leave the perf_data around. And the next session finds a non-empty data. As I said I'm in favour of removing the set_buffer() callback but I wouldn't associated it with ETR state cleanup. If the code can be rearranged in a way that code can be removed then that alone is enough to justify the change. diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 3cc4a0b..12a247d 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags) path = etm_event_cpu_path(event_data, cpu); /* We need a sink, no need to continue without one */ sink = coresight_get_sink(path); -if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer)) -goto fail_end_stop; - -/* Configure the sink */ -if (sink_ops(sink)->set_buffer(sink, handle, - event_data->snk_config)) +if (WARN_ON_ONCE(!sink)) goto fail_end_stop; /* Nothing will happen without a path */ -if (coresight_enable_path(path, CS_MODE_PERF)) +if (coresight_enable_path(path, CS_MODE_PERF, handle)) Here we already have a handle on "event_data". As such I think this is what we should feed to coresight_enable_path() rather than the handle. That way we don't need to call etm_perf_sink_config(), we just use the data. The advantage of passing on the handle is, we could get all the way upto the "perf_event" for the given session. Passing the event_data will loose that information. i.e, perf_event->
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose wrote: > > Mathieu, > > On 19/07/18 21:36, Mathieu Poirier wrote: > > On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: > >> In coresight perf mode, we need to prepare the sink before > >> starting a session, which is done via set_buffer call back. > >> We then proceed to enable the tracing. If we fail to start > >> the session successfully, we leave the sink configuration > >> unchanged. This was fine for the existing backends as they > >> don't have any state associated with the buffers. But with > >> ETR, we need to keep track of the buffer details and need > >> to be cleaned up if we fail. In order to make the operation > >> atomic and to avoid yet another call back, we get rid of > >> the "set_buffer" call back and pass the buffer details > >> via enable() call back to the sink. > > > > Suzuki, > > > > I'm not sure I understand the problem you're trying to fix there. From the > > implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the > > same result been achievable using a callback? > > We can definitely achieve the results using "set_buffer". But for ETR, > we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". > But if we failed to enable_path(), we leave the drvdata->perf_data > and doesn't clean it up. Now when another session is about to set_buf, > we check if perf_data is empty and WARNs otherwise. > Because we can't be sure if it belongs to an abandoned session or > another active session and we completely messed somewhere in the driver. > So, we need a clear_buffer call back if the enable fails, something > not really worth. Anyways, there is no point in separating set_buffer > and enabling the sink, as the error handling becomes cumbersome as explained > above. > > > > > I'm fine with this patch and supportive of getting rid of callbacks if we > > can, I > > just need to understand the exact problem you're after. From looking a your > > code (and the current implementation), if we succeed in setting the memory > > for > > the sink but fail in any of the subsequent steps i.e, enabling the rest of > > the > > compoment on the path or the source, the sink is left unchanged. > > Yes, thats right. And we should WARN (which I missed in this version) if > there is a perf_data already for a disabled ETR. Please see my response to the > next patch. The changelog for this patch states the following: "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." I did a deep dive in the code and in the current implementation if the source fails to be enabled in etm_event_start() the path and the sink remains unchanged. With your patchset this get fixed because a goto was added to disable the path when such condition occur. As such each component in the path will see its ->disable() callback invoked. In tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in tmc_etr_disable_hw(), so the cleanup on error condition is done properly. As such we wouldn't need a clean_buffer() callback. As I said I'm in favour of removing the set_buffer() callback but I wouldn't associated it with ETR state cleanup. If the code can be rearranged in a way that code can be removed then that alone is enough to justify the change. > > >> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c > >> b/drivers/hwtracing/coresight/coresight-etm-perf.c > >> index 3cc4a0b..12a247d 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > >> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > >> @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event > >> *event, int flags) > >> path = etm_event_cpu_path(event_data, cpu); > >> /* We need a sink, no need to continue without one */ > >> sink = coresight_get_sink(path); > >> -if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer)) > >> -goto fail_end_stop; > >> - > >> -/* Configure the sink */ > >> -if (sink_ops(sink)->set_buffer(sink, handle, > >> - event_data->snk_config)) > >> +if (WARN_ON_ONCE(!sink)) > >> goto fail_end_stop; > >> > >> /* Nothing will happen without a path */ > >> -if (coresight_enable_path(path, CS_MODE_PERF)) > >> +if (coresight_enable_path(path, CS_MODE_PERF, handle)) > > > > Here we already have a handle on "event_data". As such I think this is > > what we > > should feed to coresight_enable_path() rather than the handle. That way we > > don't need to call etm_perf_sink_config(), we just use the data. > > The advantage of passing on the handle is, we could get all the way upto the > "perf_event" for the given session. Passing the event_data will loose that > information. > > i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config >| <-event || | > > The purpose of the wrapper "etm_perf_sink_config()" is to abstract
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose wrote: > > Mathieu, > > On 19/07/18 21:36, Mathieu Poirier wrote: > > On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: > >> In coresight perf mode, we need to prepare the sink before > >> starting a session, which is done via set_buffer call back. > >> We then proceed to enable the tracing. If we fail to start > >> the session successfully, we leave the sink configuration > >> unchanged. This was fine for the existing backends as they > >> don't have any state associated with the buffers. But with > >> ETR, we need to keep track of the buffer details and need > >> to be cleaned up if we fail. In order to make the operation > >> atomic and to avoid yet another call back, we get rid of > >> the "set_buffer" call back and pass the buffer details > >> via enable() call back to the sink. > > > > Suzuki, > > > > I'm not sure I understand the problem you're trying to fix there. From the > > implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the > > same result been achievable using a callback? > > We can definitely achieve the results using "set_buffer". But for ETR, > we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". > But if we failed to enable_path(), we leave the drvdata->perf_data > and doesn't clean it up. Now when another session is about to set_buf, > we check if perf_data is empty and WARNs otherwise. > Because we can't be sure if it belongs to an abandoned session or > another active session and we completely messed somewhere in the driver. > So, we need a clear_buffer call back if the enable fails, something > not really worth. Anyways, there is no point in separating set_buffer > and enabling the sink, as the error handling becomes cumbersome as explained > above. > > > > > I'm fine with this patch and supportive of getting rid of callbacks if we > > can, I > > just need to understand the exact problem you're after. From looking a your > > code (and the current implementation), if we succeed in setting the memory > > for > > the sink but fail in any of the subsequent steps i.e, enabling the rest of > > the > > compoment on the path or the source, the sink is left unchanged. > > Yes, thats right. And we should WARN (which I missed in this version) if > there is a perf_data already for a disabled ETR. Please see my response to the > next patch. The changelog for this patch states the following: "But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail." I did a deep dive in the code and in the current implementation if the source fails to be enabled in etm_event_start() the path and the sink remains unchanged. With your patchset this get fixed because a goto was added to disable the path when such condition occur. As such each component in the path will see its ->disable() callback invoked. In tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in tmc_etr_disable_hw(), so the cleanup on error condition is done properly. As such we wouldn't need a clean_buffer() callback. As I said I'm in favour of removing the set_buffer() callback but I wouldn't associated it with ETR state cleanup. If the code can be rearranged in a way that code can be removed then that alone is enough to justify the change. > > >> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c > >> b/drivers/hwtracing/coresight/coresight-etm-perf.c > >> index 3cc4a0b..12a247d 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > >> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > >> @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event > >> *event, int flags) > >> path = etm_event_cpu_path(event_data, cpu); > >> /* We need a sink, no need to continue without one */ > >> sink = coresight_get_sink(path); > >> -if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer)) > >> -goto fail_end_stop; > >> - > >> -/* Configure the sink */ > >> -if (sink_ops(sink)->set_buffer(sink, handle, > >> - event_data->snk_config)) > >> +if (WARN_ON_ONCE(!sink)) > >> goto fail_end_stop; > >> > >> /* Nothing will happen without a path */ > >> -if (coresight_enable_path(path, CS_MODE_PERF)) > >> +if (coresight_enable_path(path, CS_MODE_PERF, handle)) > > > > Here we already have a handle on "event_data". As such I think this is > > what we > > should feed to coresight_enable_path() rather than the handle. That way we > > don't need to call etm_perf_sink_config(), we just use the data. > > The advantage of passing on the handle is, we could get all the way upto the > "perf_event" for the given session. Passing the event_data will loose that > information. > > i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config >| <-event || | > > The purpose of the wrapper "etm_perf_sink_config()" is to abstract
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
Mathieu, On 19/07/18 21:36, Mathieu Poirier wrote: On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: In coresight perf mode, we need to prepare the sink before starting a session, which is done via set_buffer call back. We then proceed to enable the tracing. If we fail to start the session successfully, we leave the sink configuration unchanged. This was fine for the existing backends as they don't have any state associated with the buffers. But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail. In order to make the operation atomic and to avoid yet another call back, we get rid of the "set_buffer" call back and pass the buffer details via enable() call back to the sink. Suzuki, I'm not sure I understand the problem you're trying to fix there. From the implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the same result been achievable using a callback? We can definitely achieve the results using "set_buffer". But for ETR, we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". But if we failed to enable_path(), we leave the drvdata->perf_data and doesn't clean it up. Now when another session is about to set_buf, we check if perf_data is empty and WARNs otherwise. Because we can't be sure if it belongs to an abandoned session or another active session and we completely messed somewhere in the driver. So, we need a clear_buffer call back if the enable fails, something not really worth. Anyways, there is no point in separating set_buffer and enabling the sink, as the error handling becomes cumbersome as explained above. I'm fine with this patch and supportive of getting rid of callbacks if we can, I just need to understand the exact problem you're after. From looking a your code (and the current implementation), if we succeed in setting the memory for the sink but fail in any of the subsequent steps i.e, enabling the rest of the compoment on the path or the source, the sink is left unchanged. Yes, thats right. And we should WARN (which I missed in this version) if there is a perf_data already for a disabled ETR. Please see my response to the next patch. diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 3cc4a0b..12a247d 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags) path = etm_event_cpu_path(event_data, cpu); /* We need a sink, no need to continue without one */ sink = coresight_get_sink(path); - if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer)) - goto fail_end_stop; - - /* Configure the sink */ - if (sink_ops(sink)->set_buffer(sink, handle, - event_data->snk_config)) + if (WARN_ON_ONCE(!sink)) goto fail_end_stop; /* Nothing will happen without a path */ - if (coresight_enable_path(path, CS_MODE_PERF)) + if (coresight_enable_path(path, CS_MODE_PERF, handle)) Here we already have a handle on "event_data". As such I think this is what we should feed to coresight_enable_path() rather than the handle. That way we don't need to call etm_perf_sink_config(), we just use the data. The advantage of passing on the handle is, we could get all the way upto the "perf_event" for the given session. Passing the event_data will loose that information. i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config | <-event || | The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we handle the information under the event_data. i.e, if we decide to make some changes in the way we store event_data, we need to spill the changes every where. But the perf_ouput_handle has much more stable ABI than event_data, hence the choice of passing handle. Cheers Suzuki
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
Mathieu, On 19/07/18 21:36, Mathieu Poirier wrote: On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote: In coresight perf mode, we need to prepare the sink before starting a session, which is done via set_buffer call back. We then proceed to enable the tracing. If we fail to start the session successfully, we leave the sink configuration unchanged. This was fine for the existing backends as they don't have any state associated with the buffers. But with ETR, we need to keep track of the buffer details and need to be cleaned up if we fail. In order to make the operation atomic and to avoid yet another call back, we get rid of the "set_buffer" call back and pass the buffer details via enable() call back to the sink. Suzuki, I'm not sure I understand the problem you're trying to fix there. From the implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the same result been achievable using a callback? We can definitely achieve the results using "set_buffer". But for ETR, we track the "perf_buf" in drvdata->perf_data when we do "set_buffer". But if we failed to enable_path(), we leave the drvdata->perf_data and doesn't clean it up. Now when another session is about to set_buf, we check if perf_data is empty and WARNs otherwise. Because we can't be sure if it belongs to an abandoned session or another active session and we completely messed somewhere in the driver. So, we need a clear_buffer call back if the enable fails, something not really worth. Anyways, there is no point in separating set_buffer and enabling the sink, as the error handling becomes cumbersome as explained above. I'm fine with this patch and supportive of getting rid of callbacks if we can, I just need to understand the exact problem you're after. From looking a your code (and the current implementation), if we succeed in setting the memory for the sink but fail in any of the subsequent steps i.e, enabling the rest of the compoment on the path or the source, the sink is left unchanged. Yes, thats right. And we should WARN (which I missed in this version) if there is a perf_data already for a disabled ETR. Please see my response to the next patch. diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 3cc4a0b..12a247d 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags) path = etm_event_cpu_path(event_data, cpu); /* We need a sink, no need to continue without one */ sink = coresight_get_sink(path); - if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer)) - goto fail_end_stop; - - /* Configure the sink */ - if (sink_ops(sink)->set_buffer(sink, handle, - event_data->snk_config)) + if (WARN_ON_ONCE(!sink)) goto fail_end_stop; /* Nothing will happen without a path */ - if (coresight_enable_path(path, CS_MODE_PERF)) + if (coresight_enable_path(path, CS_MODE_PERF, handle)) Here we already have a handle on "event_data". As such I think this is what we should feed to coresight_enable_path() rather than the handle. That way we don't need to call etm_perf_sink_config(), we just use the data. The advantage of passing on the handle is, we could get all the way upto the "perf_event" for the given session. Passing the event_data will loose that information. i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config | <-event || | The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we handle the information under the event_data. i.e, if we decide to make some changes in the way we store event_data, we need to spill the changes every where. But the perf_ouput_handle has much more stable ABI than event_data, hence the choice of passing handle. Cheers Suzuki
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
> In coresight perf mode, we need to prepare the sink before
> starting a session, which is done via set_buffer call back.
> We then proceed to enable the tracing. If we fail to start
> the session successfully, we leave the sink configuration
> unchanged. This was fine for the existing backends as they
> don't have any state associated with the buffers. But with
> ETR, we need to keep track of the buffer details and need
> to be cleaned up if we fail. In order to make the operation
> atomic and to avoid yet another call back, we get rid of
> the "set_buffer" call back and pass the buffer details
> via enable() call back to the sink.
Suzuki,
I'm not sure I understand the problem you're trying to fix there. From the
implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
same result been achievable using a callback?
I'm fine with this patch and supportive of getting rid of callbacks if we can, I
just need to understand the exact problem you're after. From looking a your
code (and the current implementation), if we succeed in setting the memory for
the sink but fail in any of the subsequent steps i.e, enabling the rest of the
compoment on the path or the source, the sink is left unchanged.
> Cc: Mathieu Poirier
> Signed-off-by: Suzuki K Poulose
> ---
> drivers/hwtracing/coresight/coresight-etb10.c| 24 ++--
> drivers/hwtracing/coresight/coresight-etm-perf.c | 9 ++--
> drivers/hwtracing/coresight/coresight-priv.h | 2 +-
> drivers/hwtracing/coresight/coresight-tmc-etf.c | 28
>
> drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 +++---
> drivers/hwtracing/coresight/coresight-tpiu.c | 2 +-
> drivers/hwtracing/coresight/coresight.c | 11 +-
> include/linux/coresight.h| 6 +
> 8 files changed, 51 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c
> b/drivers/hwtracing/coresight/coresight-etb10.c
> index a729a7b..4e829db 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -28,6 +28,7 @@
>
>
> #include "coresight-priv.h"
> +#include "coresight-etm-perf.h"
>
> #define ETB_RAM_DEPTH_REG0x004
> #define ETB_STATUS_REG 0x00c
> @@ -90,6 +91,9 @@ struct etb_drvdata {
> u32 trigger_cntr;
> };
>
> +static int etb_set_buffer(struct coresight_device *csdev,
> + struct perf_output_handle *handle);
> +
> static unsigned int etb_get_buffer_depth(struct etb_drvdata *drvdata)
> {
> u32 depth = 0;
> @@ -131,12 +135,19 @@ static void etb_enable_hw(struct etb_drvdata *drvdata)
> CS_LOCK(drvdata->base);
> }
>
> -static int etb_enable(struct coresight_device *csdev, u32 mode)
> +static int etb_enable(struct coresight_device *csdev, u32 mode, void *data)
> {
> + int ret = 0;
> u32 val;
> unsigned long flags;
> struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> + if (mode == CS_MODE_PERF) {
> + ret = etb_set_buffer(csdev, (struct perf_output_handle *)data);
> + if (ret)
> + goto out;
> + }
> +
> val = local_cmpxchg(>mode,
> CS_MODE_DISABLED, mode);
> /*
> @@ -156,8 +167,9 @@ static int etb_enable(struct coresight_device *csdev, u32
> mode)
> spin_unlock_irqrestore(>spinlock, flags);
>
> out:
> - dev_dbg(drvdata->dev, "ETB enabled\n");
> - return 0;
> + if (!ret)
> + dev_dbg(drvdata->dev, "ETB enabled\n");
> + return ret;
> }
>
> static void etb_disable_hw(struct etb_drvdata *drvdata)
> @@ -294,12 +306,11 @@ static void etb_free_buffer(void *config)
> }
>
> static int etb_set_buffer(struct coresight_device *csdev,
> - struct perf_output_handle *handle,
> - void *sink_config)
> + struct perf_output_handle *handle)
> {
> int ret = 0;
> unsigned long head;
> - struct cs_buffers *buf = sink_config;
> + struct cs_buffers *buf = etm_perf_sink_config(handle);
>
> /* wrap head around to the amount of space we have */
> head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
> @@ -453,7 +464,6 @@ static const struct coresight_ops_sink etb_sink_ops = {
> .disable= etb_disable,
> .alloc_buffer = etb_alloc_buffer,
> .free_buffer= etb_free_buffer,
> - .set_buffer = etb_set_buffer,
> .update_buffer = etb_update_buffer,
> };
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
> b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 3cc4a0b..12a247d 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -269,16 +269,11 @@
Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
> In coresight perf mode, we need to prepare the sink before
> starting a session, which is done via set_buffer call back.
> We then proceed to enable the tracing. If we fail to start
> the session successfully, we leave the sink configuration
> unchanged. This was fine for the existing backends as they
> don't have any state associated with the buffers. But with
> ETR, we need to keep track of the buffer details and need
> to be cleaned up if we fail. In order to make the operation
> atomic and to avoid yet another call back, we get rid of
> the "set_buffer" call back and pass the buffer details
> via enable() call back to the sink.
Suzuki,
I'm not sure I understand the problem you're trying to fix there. From the
implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
same result been achievable using a callback?
I'm fine with this patch and supportive of getting rid of callbacks if we can, I
just need to understand the exact problem you're after. From looking a your
code (and the current implementation), if we succeed in setting the memory for
the sink but fail in any of the subsequent steps i.e, enabling the rest of the
compoment on the path or the source, the sink is left unchanged.
> Cc: Mathieu Poirier
> Signed-off-by: Suzuki K Poulose
> ---
> drivers/hwtracing/coresight/coresight-etb10.c| 24 ++--
> drivers/hwtracing/coresight/coresight-etm-perf.c | 9 ++--
> drivers/hwtracing/coresight/coresight-priv.h | 2 +-
> drivers/hwtracing/coresight/coresight-tmc-etf.c | 28
>
> drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 +++---
> drivers/hwtracing/coresight/coresight-tpiu.c | 2 +-
> drivers/hwtracing/coresight/coresight.c | 11 +-
> include/linux/coresight.h| 6 +
> 8 files changed, 51 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c
> b/drivers/hwtracing/coresight/coresight-etb10.c
> index a729a7b..4e829db 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -28,6 +28,7 @@
>
>
> #include "coresight-priv.h"
> +#include "coresight-etm-perf.h"
>
> #define ETB_RAM_DEPTH_REG0x004
> #define ETB_STATUS_REG 0x00c
> @@ -90,6 +91,9 @@ struct etb_drvdata {
> u32 trigger_cntr;
> };
>
> +static int etb_set_buffer(struct coresight_device *csdev,
> + struct perf_output_handle *handle);
> +
> static unsigned int etb_get_buffer_depth(struct etb_drvdata *drvdata)
> {
> u32 depth = 0;
> @@ -131,12 +135,19 @@ static void etb_enable_hw(struct etb_drvdata *drvdata)
> CS_LOCK(drvdata->base);
> }
>
> -static int etb_enable(struct coresight_device *csdev, u32 mode)
> +static int etb_enable(struct coresight_device *csdev, u32 mode, void *data)
> {
> + int ret = 0;
> u32 val;
> unsigned long flags;
> struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> + if (mode == CS_MODE_PERF) {
> + ret = etb_set_buffer(csdev, (struct perf_output_handle *)data);
> + if (ret)
> + goto out;
> + }
> +
> val = local_cmpxchg(>mode,
> CS_MODE_DISABLED, mode);
> /*
> @@ -156,8 +167,9 @@ static int etb_enable(struct coresight_device *csdev, u32
> mode)
> spin_unlock_irqrestore(>spinlock, flags);
>
> out:
> - dev_dbg(drvdata->dev, "ETB enabled\n");
> - return 0;
> + if (!ret)
> + dev_dbg(drvdata->dev, "ETB enabled\n");
> + return ret;
> }
>
> static void etb_disable_hw(struct etb_drvdata *drvdata)
> @@ -294,12 +306,11 @@ static void etb_free_buffer(void *config)
> }
>
> static int etb_set_buffer(struct coresight_device *csdev,
> - struct perf_output_handle *handle,
> - void *sink_config)
> + struct perf_output_handle *handle)
> {
> int ret = 0;
> unsigned long head;
> - struct cs_buffers *buf = sink_config;
> + struct cs_buffers *buf = etm_perf_sink_config(handle);
>
> /* wrap head around to the amount of space we have */
> head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
> @@ -453,7 +464,6 @@ static const struct coresight_ops_sink etb_sink_ops = {
> .disable= etb_disable,
> .alloc_buffer = etb_alloc_buffer,
> .free_buffer= etb_free_buffer,
> - .set_buffer = etb_set_buffer,
> .update_buffer = etb_update_buffer,
> };
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
> b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 3cc4a0b..12a247d 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -269,16 +269,11 @@
[PATCH v2 09/10] coresight: perf: Remove set_buffer call back
In coresight perf mode, we need to prepare the sink before
starting a session, which is done via set_buffer call back.
We then proceed to enable the tracing. If we fail to start
the session successfully, we leave the sink configuration
unchanged. This was fine for the existing backends as they
don't have any state associated with the buffers. But with
ETR, we need to keep track of the buffer details and need
to be cleaned up if we fail. In order to make the operation
atomic and to avoid yet another call back, we get rid of
the "set_buffer" call back and pass the buffer details
via enable() call back to the sink.
Cc: Mathieu Poirier
Signed-off-by: Suzuki K Poulose
---
drivers/hwtracing/coresight/coresight-etb10.c| 24 ++--
drivers/hwtracing/coresight/coresight-etm-perf.c | 9 ++--
drivers/hwtracing/coresight/coresight-priv.h | 2 +-
drivers/hwtracing/coresight/coresight-tmc-etf.c | 28
drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 +++---
drivers/hwtracing/coresight/coresight-tpiu.c | 2 +-
drivers/hwtracing/coresight/coresight.c | 11 +-
include/linux/coresight.h| 6 +
8 files changed, 51 insertions(+), 38 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c
b/drivers/hwtracing/coresight/coresight-etb10.c
index a729a7b..4e829db 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -28,6 +28,7 @@
#include "coresight-priv.h"
+#include "coresight-etm-perf.h"
#define ETB_RAM_DEPTH_REG 0x004
#define ETB_STATUS_REG 0x00c
@@ -90,6 +91,9 @@ struct etb_drvdata {
u32 trigger_cntr;
};
+static int etb_set_buffer(struct coresight_device *csdev,
+ struct perf_output_handle *handle);
+
static unsigned int etb_get_buffer_depth(struct etb_drvdata *drvdata)
{
u32 depth = 0;
@@ -131,12 +135,19 @@ static void etb_enable_hw(struct etb_drvdata *drvdata)
CS_LOCK(drvdata->base);
}
-static int etb_enable(struct coresight_device *csdev, u32 mode)
+static int etb_enable(struct coresight_device *csdev, u32 mode, void *data)
{
+ int ret = 0;
u32 val;
unsigned long flags;
struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ if (mode == CS_MODE_PERF) {
+ ret = etb_set_buffer(csdev, (struct perf_output_handle *)data);
+ if (ret)
+ goto out;
+ }
+
val = local_cmpxchg(>mode,
CS_MODE_DISABLED, mode);
/*
@@ -156,8 +167,9 @@ static int etb_enable(struct coresight_device *csdev, u32
mode)
spin_unlock_irqrestore(>spinlock, flags);
out:
- dev_dbg(drvdata->dev, "ETB enabled\n");
- return 0;
+ if (!ret)
+ dev_dbg(drvdata->dev, "ETB enabled\n");
+ return ret;
}
static void etb_disable_hw(struct etb_drvdata *drvdata)
@@ -294,12 +306,11 @@ static void etb_free_buffer(void *config)
}
static int etb_set_buffer(struct coresight_device *csdev,
- struct perf_output_handle *handle,
- void *sink_config)
+ struct perf_output_handle *handle)
{
int ret = 0;
unsigned long head;
- struct cs_buffers *buf = sink_config;
+ struct cs_buffers *buf = etm_perf_sink_config(handle);
/* wrap head around to the amount of space we have */
head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
@@ -453,7 +464,6 @@ static const struct coresight_ops_sink etb_sink_ops = {
.disable= etb_disable,
.alloc_buffer = etb_alloc_buffer,
.free_buffer= etb_free_buffer,
- .set_buffer = etb_set_buffer,
.update_buffer = etb_update_buffer,
};
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 3cc4a0b..12a247d 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int
flags)
path = etm_event_cpu_path(event_data, cpu);
/* We need a sink, no need to continue without one */
sink = coresight_get_sink(path);
- if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
- goto fail_end_stop;
-
- /* Configure the sink */
- if (sink_ops(sink)->set_buffer(sink, handle,
- event_data->snk_config))
+ if (WARN_ON_ONCE(!sink))
goto fail_end_stop;
/* Nothing will happen without a path */
- if (coresight_enable_path(path, CS_MODE_PERF))
+ if (coresight_enable_path(path, CS_MODE_PERF, handle))
goto fail_end_stop;
/* Tell the perf core the event is alive */
diff --git
[PATCH v2 09/10] coresight: perf: Remove set_buffer call back
In coresight perf mode, we need to prepare the sink before
starting a session, which is done via set_buffer call back.
We then proceed to enable the tracing. If we fail to start
the session successfully, we leave the sink configuration
unchanged. This was fine for the existing backends as they
don't have any state associated with the buffers. But with
ETR, we need to keep track of the buffer details and need
to be cleaned up if we fail. In order to make the operation
atomic and to avoid yet another call back, we get rid of
the "set_buffer" call back and pass the buffer details
via enable() call back to the sink.
Cc: Mathieu Poirier
Signed-off-by: Suzuki K Poulose
---
drivers/hwtracing/coresight/coresight-etb10.c| 24 ++--
drivers/hwtracing/coresight/coresight-etm-perf.c | 9 ++--
drivers/hwtracing/coresight/coresight-priv.h | 2 +-
drivers/hwtracing/coresight/coresight-tmc-etf.c | 28
drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 +++---
drivers/hwtracing/coresight/coresight-tpiu.c | 2 +-
drivers/hwtracing/coresight/coresight.c | 11 +-
include/linux/coresight.h| 6 +
8 files changed, 51 insertions(+), 38 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c
b/drivers/hwtracing/coresight/coresight-etb10.c
index a729a7b..4e829db 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -28,6 +28,7 @@
#include "coresight-priv.h"
+#include "coresight-etm-perf.h"
#define ETB_RAM_DEPTH_REG 0x004
#define ETB_STATUS_REG 0x00c
@@ -90,6 +91,9 @@ struct etb_drvdata {
u32 trigger_cntr;
};
+static int etb_set_buffer(struct coresight_device *csdev,
+ struct perf_output_handle *handle);
+
static unsigned int etb_get_buffer_depth(struct etb_drvdata *drvdata)
{
u32 depth = 0;
@@ -131,12 +135,19 @@ static void etb_enable_hw(struct etb_drvdata *drvdata)
CS_LOCK(drvdata->base);
}
-static int etb_enable(struct coresight_device *csdev, u32 mode)
+static int etb_enable(struct coresight_device *csdev, u32 mode, void *data)
{
+ int ret = 0;
u32 val;
unsigned long flags;
struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ if (mode == CS_MODE_PERF) {
+ ret = etb_set_buffer(csdev, (struct perf_output_handle *)data);
+ if (ret)
+ goto out;
+ }
+
val = local_cmpxchg(>mode,
CS_MODE_DISABLED, mode);
/*
@@ -156,8 +167,9 @@ static int etb_enable(struct coresight_device *csdev, u32
mode)
spin_unlock_irqrestore(>spinlock, flags);
out:
- dev_dbg(drvdata->dev, "ETB enabled\n");
- return 0;
+ if (!ret)
+ dev_dbg(drvdata->dev, "ETB enabled\n");
+ return ret;
}
static void etb_disable_hw(struct etb_drvdata *drvdata)
@@ -294,12 +306,11 @@ static void etb_free_buffer(void *config)
}
static int etb_set_buffer(struct coresight_device *csdev,
- struct perf_output_handle *handle,
- void *sink_config)
+ struct perf_output_handle *handle)
{
int ret = 0;
unsigned long head;
- struct cs_buffers *buf = sink_config;
+ struct cs_buffers *buf = etm_perf_sink_config(handle);
/* wrap head around to the amount of space we have */
head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
@@ -453,7 +464,6 @@ static const struct coresight_ops_sink etb_sink_ops = {
.disable= etb_disable,
.alloc_buffer = etb_alloc_buffer,
.free_buffer= etb_free_buffer,
- .set_buffer = etb_set_buffer,
.update_buffer = etb_update_buffer,
};
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 3cc4a0b..12a247d 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int
flags)
path = etm_event_cpu_path(event_data, cpu);
/* We need a sink, no need to continue without one */
sink = coresight_get_sink(path);
- if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
- goto fail_end_stop;
-
- /* Configure the sink */
- if (sink_ops(sink)->set_buffer(sink, handle,
- event_data->snk_config))
+ if (WARN_ON_ONCE(!sink))
goto fail_end_stop;
/* Nothing will happen without a path */
- if (coresight_enable_path(path, CS_MODE_PERF))
+ if (coresight_enable_path(path, CS_MODE_PERF, handle))
goto fail_end_stop;
/* Tell the perf core the event is alive */
diff --git
