* [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void.
@ 2015-09-06 8:52 Dave Airlie
2015-09-06 8:53 ` [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2) Dave Airlie
2015-09-30 7:33 ` [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void Daniel Vetter
0 siblings, 2 replies; 5+ messages in thread
From: Dave Airlie @ 2015-09-06 8:52 UTC (permalink / raw)
To: dri-devel
This just drops the return and assigning it to a len variable
that never gets used.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index e23df5f..294d904 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -631,7 +631,7 @@ static bool drm_dp_sideband_parse_req(struct drm_dp_sideband_msg_rx *raw,
}
}
-static int build_dpcd_write(struct drm_dp_sideband_msg_tx *msg, u8 port_num, u32 offset, u8 num_bytes, u8 *bytes)
+static void build_dpcd_write(struct drm_dp_sideband_msg_tx *msg, u8 port_num, u32 offset, u8 num_bytes, u8 *bytes)
{
struct drm_dp_sideband_msg_req_body req;
@@ -641,20 +641,17 @@ static int build_dpcd_write(struct drm_dp_sideband_msg_tx *msg, u8 port_num, u32
req.u.dpcd_write.num_bytes = num_bytes;
req.u.dpcd_write.bytes = bytes;
drm_dp_encode_sideband_req(&req, msg);
-
- return 0;
}
-static int build_link_address(struct drm_dp_sideband_msg_tx *msg)
+static void build_link_address(struct drm_dp_sideband_msg_tx *msg)
{
struct drm_dp_sideband_msg_req_body req;
req.req_type = DP_LINK_ADDRESS;
drm_dp_encode_sideband_req(&req, msg);
- return 0;
}
-static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int port_num)
+static void build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int port_num)
{
struct drm_dp_sideband_msg_req_body req;
@@ -662,10 +659,9 @@ static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int por
req.u.port_num.port_number = port_num;
drm_dp_encode_sideband_req(&req, msg);
msg->path_msg = true;
- return 0;
}
-static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_num,
+static void build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_num,
u8 vcpi, uint16_t pbn)
{
struct drm_dp_sideband_msg_req_body req;
@@ -676,7 +672,6 @@ static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_n
req.u.allocate_payload.pbn = pbn;
drm_dp_encode_sideband_req(&req, msg);
msg->path_msg = true;
- return 0;
}
static int drm_dp_mst_assign_payload_id(struct drm_dp_mst_topology_mgr *mgr,
@@ -1461,7 +1456,6 @@ static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_branch *mstb)
{
- int len;
struct drm_dp_sideband_msg_tx *txmsg;
int ret;
@@ -1470,7 +1464,7 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
return -ENOMEM;
txmsg->dst = mstb;
- len = build_link_address(txmsg);
+ build_link_address(txmsg);
drm_dp_queue_down_tx(mgr, txmsg);
@@ -1510,7 +1504,6 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_branch *mstb,
struct drm_dp_mst_port *port)
{
- int len;
struct drm_dp_sideband_msg_tx *txmsg;
int ret;
@@ -1519,7 +1512,7 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
return -ENOMEM;
txmsg->dst = mstb;
- len = build_enum_path_resources(txmsg, port->port_num);
+ build_enum_path_resources(txmsg, port->port_num);
drm_dp_queue_down_tx(mgr, txmsg);
@@ -1547,7 +1540,7 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
{
struct drm_dp_sideband_msg_tx *txmsg;
struct drm_dp_mst_branch *mstb;
- int len, ret;
+ int ret;
mstb = drm_dp_get_validated_mstb_ref(mgr, port->parent);
if (!mstb)
@@ -1560,9 +1553,8 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
}
txmsg->dst = mstb;
- len = build_allocate_payload(txmsg, port->port_num,
- id,
- pbn);
+ build_allocate_payload(txmsg, port->port_num,
+ id, pbn);
drm_dp_queue_down_tx(mgr, txmsg);
@@ -1772,7 +1764,6 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port,
int offset, int size, u8 *bytes)
{
- int len;
int ret;
struct drm_dp_sideband_msg_tx *txmsg;
struct drm_dp_mst_branch *mstb;
@@ -1787,7 +1778,7 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
goto fail_put;
}
- len = build_dpcd_write(txmsg, port->port_num, offset, size, bytes);
+ build_dpcd_write(txmsg, port->port_num, offset, size, bytes);
txmsg->dst = mstb;
drm_dp_queue_down_tx(mgr, txmsg);
--
2.4.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2)
2015-09-06 8:52 [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void Dave Airlie
@ 2015-09-06 8:53 ` Dave Airlie
2015-09-30 7:44 ` Daniel Vetter
2015-09-30 7:33 ` [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void Daniel Vetter
1 sibling, 1 reply; 5+ messages in thread
From: Dave Airlie @ 2015-09-06 8:53 UTC (permalink / raw)
To: dri-devel
There is a race where the reply could get processed by another
work queue before we've updated the state.
Update the state before sending the msg to close it.
v2: reset value if return indicates we haven't send the msg.
Pointed out by Adam J Richter on
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91481
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 294d904..831096b9 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1101,8 +1101,10 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
ret = drm_dp_port_setup_pdt(port);
if (ret == true) {
- drm_dp_send_link_address(mstb->mgr, port->mstb);
port->mstb->link_address_sent = true;
+ ret = drm_dp_send_link_address(mstb->mgr, port->mstb);
+ if (ret < 0)
+ port->mstb->link_address_sent = false;
}
}
@@ -1198,8 +1200,11 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m
struct drm_dp_mst_port *port;
struct drm_dp_mst_branch *mstb_child;
if (!mstb->link_address_sent) {
- drm_dp_send_link_address(mgr, mstb);
+ int ret;
mstb->link_address_sent = true;
+ ret = drm_dp_send_link_address(mgr, mstb);
+ if (ret < 0)
+ mstb->link_address_sent = false;
}
list_for_each_entry(port, &mstb->ports, next) {
if (port->input)
@@ -1493,11 +1498,12 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
}
(*mgr->cbs->hotplug)(mgr);
}
+ ret = 0;
} else
DRM_DEBUG_KMS("link address failed %d\n", ret);
kfree(txmsg);
- return 0;
+ return ret;
}
static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
--
2.4.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2)
2015-09-06 8:53 ` [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2) Dave Airlie
@ 2015-09-30 7:44 ` Daniel Vetter
2015-10-01 1:48 ` Dave Airlie
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2015-09-30 7:44 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Sun, Sep 06, 2015 at 06:53:00PM +1000, Dave Airlie wrote:
> There is a race where the reply could get processed by another
> work queue before we've updated the state.
>
> Update the state before sending the msg to close it.
>
> v2: reset value if return indicates we haven't send the msg.
>
> Pointed out by Adam J Richter on
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91481
> Signed-off-by: Dave Airlie <airlied@redhat.com>
Ok we see to have two callers of drm_dp_send_link_address:
- drm_dp_add_port, but the only caller of that in turn is again
drm_dp_send_link_address. This is just self-recursion.
- drm_dp_check_and_send_link_address, which in turn is called only by
itself and by drm_dp_mst_link_probe_work.
drm_dp_mst_link_probe_work is only called from the mgr->work function, and
a single work item is never run concurrently.
I don't see any race here at all. A good cleanup though would be what
Adam's original patch has done and move the link_address_sent = true into
drm_dp_send_link_address.
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 294d904..831096b9 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1101,8 +1101,10 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
>
> ret = drm_dp_port_setup_pdt(port);
> if (ret == true) {
> - drm_dp_send_link_address(mstb->mgr, port->mstb);
> port->mstb->link_address_sent = true;
> + ret = drm_dp_send_link_address(mstb->mgr, port->mstb);
> + if (ret < 0)
> + port->mstb->link_address_sent = false;
> }
> }
>
> @@ -1198,8 +1200,11 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m
> struct drm_dp_mst_port *port;
> struct drm_dp_mst_branch *mstb_child;
> if (!mstb->link_address_sent) {
> - drm_dp_send_link_address(mgr, mstb);
> + int ret;
> mstb->link_address_sent = true;
> + ret = drm_dp_send_link_address(mgr, mstb);
> + if (ret < 0)
> + mstb->link_address_sent = false;
> }
> list_for_each_entry(port, &mstb->ports, next) {
> if (port->input)
> @@ -1493,11 +1498,12 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
> }
> (*mgr->cbs->hotplug)(mgr);
> }
> + ret = 0;
> } else
> DRM_DEBUG_KMS("link address failed %d\n", ret);
>
> kfree(txmsg);
> - return 0;
> + return ret;
> }
>
> static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
> --
> 2.4.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2)
2015-09-30 7:44 ` Daniel Vetter
@ 2015-10-01 1:48 ` Dave Airlie
0 siblings, 0 replies; 5+ messages in thread
From: Dave Airlie @ 2015-10-01 1:48 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
On 30 September 2015 at 17:44, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Sep 06, 2015 at 06:53:00PM +1000, Dave Airlie wrote:
>> There is a race where the reply could get processed by another
>> work queue before we've updated the state.
>>
>> Update the state before sending the msg to close it.
>>
>> v2: reset value if return indicates we haven't send the msg.
>>
>> Pointed out by Adam J Richter on
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91481
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>
> Ok we see to have two callers of drm_dp_send_link_address:
> - drm_dp_add_port, but the only caller of that in turn is again
> drm_dp_send_link_address. This is just self-recursion.
> - drm_dp_check_and_send_link_address, which in turn is called only by
> itself and by drm_dp_mst_link_probe_work.
>
> drm_dp_mst_link_probe_work is only called from the mgr->work function, and
> a single work item is never run concurrently.
>
> I don't see any race here at all. A good cleanup though would be what
> Adam's original patch has done and move the link_address_sent = true into
> drm_dp_send_link_address.
I think we were being paranoid about the work queue vs the send_link_address
if you can't have multiple then it matters not.
But I'll repost the cleaned up version as it makes things easier to grok.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void.
2015-09-06 8:52 [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void Dave Airlie
2015-09-06 8:53 ` [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2) Dave Airlie
@ 2015-09-30 7:33 ` Daniel Vetter
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2015-09-30 7:33 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Sun, Sep 06, 2015 at 06:52:59PM +1000, Dave Airlie wrote:
> This just drops the return and assigning it to a len variable
> that never gets used.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 29 ++++++++++-------------------
> 1 file changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index e23df5f..294d904 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -631,7 +631,7 @@ static bool drm_dp_sideband_parse_req(struct drm_dp_sideband_msg_rx *raw,
> }
> }
>
> -static int build_dpcd_write(struct drm_dp_sideband_msg_tx *msg, u8 port_num, u32 offset, u8 num_bytes, u8 *bytes)
> +static void build_dpcd_write(struct drm_dp_sideband_msg_tx *msg, u8 port_num, u32 offset, u8 num_bytes, u8 *bytes)
> {
> struct drm_dp_sideband_msg_req_body req;
>
> @@ -641,20 +641,17 @@ static int build_dpcd_write(struct drm_dp_sideband_msg_tx *msg, u8 port_num, u32
> req.u.dpcd_write.num_bytes = num_bytes;
> req.u.dpcd_write.bytes = bytes;
> drm_dp_encode_sideband_req(&req, msg);
> -
> - return 0;
> }
>
> -static int build_link_address(struct drm_dp_sideband_msg_tx *msg)
> +static void build_link_address(struct drm_dp_sideband_msg_tx *msg)
> {
> struct drm_dp_sideband_msg_req_body req;
>
> req.req_type = DP_LINK_ADDRESS;
> drm_dp_encode_sideband_req(&req, msg);
> - return 0;
> }
>
> -static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int port_num)
> +static void build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int port_num)
> {
> struct drm_dp_sideband_msg_req_body req;
>
> @@ -662,10 +659,9 @@ static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int por
> req.u.port_num.port_number = port_num;
> drm_dp_encode_sideband_req(&req, msg);
> msg->path_msg = true;
> - return 0;
> }
>
> -static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_num,
> +static void build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_num,
> u8 vcpi, uint16_t pbn)
> {
> struct drm_dp_sideband_msg_req_body req;
> @@ -676,7 +672,6 @@ static int build_allocate_payload(struct drm_dp_sideband_msg_tx *msg, int port_n
> req.u.allocate_payload.pbn = pbn;
> drm_dp_encode_sideband_req(&req, msg);
> msg->path_msg = true;
> - return 0;
> }
>
> static int drm_dp_mst_assign_payload_id(struct drm_dp_mst_topology_mgr *mgr,
> @@ -1461,7 +1456,6 @@ static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
> static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_branch *mstb)
> {
> - int len;
> struct drm_dp_sideband_msg_tx *txmsg;
> int ret;
>
> @@ -1470,7 +1464,7 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
> return -ENOMEM;
>
> txmsg->dst = mstb;
> - len = build_link_address(txmsg);
> + build_link_address(txmsg);
>
> drm_dp_queue_down_tx(mgr, txmsg);
>
> @@ -1510,7 +1504,6 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_branch *mstb,
> struct drm_dp_mst_port *port)
> {
> - int len;
> struct drm_dp_sideband_msg_tx *txmsg;
> int ret;
>
> @@ -1519,7 +1512,7 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
> return -ENOMEM;
>
> txmsg->dst = mstb;
> - len = build_enum_path_resources(txmsg, port->port_num);
> + build_enum_path_resources(txmsg, port->port_num);
>
> drm_dp_queue_down_tx(mgr, txmsg);
>
> @@ -1547,7 +1540,7 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
> {
> struct drm_dp_sideband_msg_tx *txmsg;
> struct drm_dp_mst_branch *mstb;
> - int len, ret;
> + int ret;
>
> mstb = drm_dp_get_validated_mstb_ref(mgr, port->parent);
> if (!mstb)
> @@ -1560,9 +1553,8 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
> }
>
> txmsg->dst = mstb;
> - len = build_allocate_payload(txmsg, port->port_num,
> - id,
> - pbn);
> + build_allocate_payload(txmsg, port->port_num,
> + id, pbn);
>
> drm_dp_queue_down_tx(mgr, txmsg);
>
> @@ -1772,7 +1764,6 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_port *port,
> int offset, int size, u8 *bytes)
> {
> - int len;
> int ret;
> struct drm_dp_sideband_msg_tx *txmsg;
> struct drm_dp_mst_branch *mstb;
> @@ -1787,7 +1778,7 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
> goto fail_put;
> }
>
> - len = build_dpcd_write(txmsg, port->port_num, offset, size, bytes);
> + build_dpcd_write(txmsg, port->port_num, offset, size, bytes);
> txmsg->dst = mstb;
>
> drm_dp_queue_down_tx(mgr, txmsg);
> --
> 2.4.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-01 1:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-06 8:52 [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void Dave Airlie
2015-09-06 8:53 ` [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2) Dave Airlie
2015-09-30 7:44 ` Daniel Vetter
2015-10-01 1:48 ` Dave Airlie
2015-09-30 7:33 ` [PATCH 1/2] drm/dp_mst: make functions that always return 0 return void Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.