* Re: [PATCH v4 04/10] net/ps3_gelic: Add new macro BUG_ON_DEBUG
From: Christophe Leroy @ 2021-08-05 5:07 UTC (permalink / raw)
To: Geoff Levand, David S. Miller, Jakub Kicinski; +Cc: netdev, linuxppc-dev
In-Reply-To: <bc659850d4eec3b2358c1ccb0e00952ceaa6012f.1627068552.git.geoff@infradead.org>
Le 23/07/2021 à 22:31, Geoff Levand a écrit :
> Add a new preprocessor macro BUG_ON_DEBUG, that expands to BUG_ON when
> the preprocessor macro DEBUG is defined, or to WARN_ON when DEBUG is not
> defined. Also, replace all occurrences of BUG_ON with BUG_ON_DEBUG.
>
CHECK:MACRO_ARG_REUSE: Macro argument reuse '_cond' - possible side-effects?
#23: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:47:
+#define BUG_ON_DEBUG(_cond) do { \
+ if (__is_defined(DEBUG)) \
+ BUG_ON(_cond); \
+ else \
+ WARN_ON(_cond); \
+} while (0)
WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG()
or BUG_ON()
#25: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:49:
+ BUG_ON(_cond); \
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Commit e4fbd62abdcd ("net/ps3_gelic: Add new macro BUG_ON_DEBUG") has style problems, please review.
NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH
EMAIL_SUBJECT FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index ded467d81f36..946e9bfa071b 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -44,6 +44,13 @@ MODULE_AUTHOR("SCE Inc.");
> MODULE_DESCRIPTION("Gelic Network driver");
> MODULE_LICENSE("GPL");
>
> +#define BUG_ON_DEBUG(_cond) do { \
> + if (__is_defined(DEBUG)) \
> + BUG_ON(_cond); \
> + else \
> + WARN_ON(_cond); \
> +} while (0)
> +
> int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
> {
> struct device *dev = ctodev(card);
> @@ -505,7 +512,7 @@ static void gelic_descr_release_tx(struct gelic_card *card,
> struct sk_buff *skb = descr->skb;
> struct device *dev = ctodev(card);
>
> - BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status) &
> + BUG_ON_DEBUG(!(be32_to_cpu(descr->hw_regs.data_status) &
> GELIC_DESCR_TX_TAIL));
>
> dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> @@ -1667,7 +1674,7 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
> }
>
> if (card->vlan[GELIC_PORT_ETHERNET_0].tx) {
> - BUG_ON(!card->vlan[GELIC_PORT_WIRELESS].tx);
> + BUG_ON_DEBUG(!card->vlan[GELIC_PORT_WIRELESS].tx);
> card->vlan_required = 1;
> } else
> card->vlan_required = 0;
> @@ -1709,7 +1716,7 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> if (result) {
> dev_err(dev, "%s:%d: ps3_dma_region_create failed: %d\n",
> __func__, __LINE__, result);
> - BUG_ON("check region type");
> + BUG_ON_DEBUG("check region type");
> goto fail_dma_region;
> }
>
>
^ permalink raw reply
* Re: [PATCH v4 03/10] net/ps3_gelic: Format cleanups
From: Christophe Leroy @ 2021-08-05 5:06 UTC (permalink / raw)
To: Geoff Levand, David S. Miller, Jakub Kicinski; +Cc: netdev, linuxppc-dev
In-Reply-To: <56efff53fcf563a1741904ea0f078d50c378b6cc.1627068552.git.geoff@infradead.org>
Le 23/07/2021 à 22:31, Geoff Levand a écrit :
> In an effort to make the PS3 gelic driver easier to maintain, cleanup the
> the driver source file formatting to be more consistent.
WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG()
or BUG_ON()
#268: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:508:
+ BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status) &
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#274: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:512:
+ dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
+ skb->len, DMA_TO_DEVICE);
WARNING:BRACES: braces {} are not necessary for single statement blocks
#295: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:580:
+ if (!stop) {
goto out;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#306: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:589:
+ if (!stop && release) {
gelic_card_wake_queues(card);
+ }
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#405: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:753:
+ pr_debug("%s:%d: hd=%d c=%ud\n", __func__, __LINE__,
+ skb_headroom(skb), c);
WARNING:BRACES: braces {} are not necessary for single statement blocks
#428: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:792:
+ if (!skb_tmp) {
return -ENOMEM;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#515: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1093:
+ if (dmac_chain_ended) {
gelic_card_enable_rxdmac(card);
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#526: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1114:
+ if (!gelic_card_decode_one_descr(card)) {
break;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#546: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1139:
+ if (!status) {
return IRQ_NONE;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#557: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1160:
+ if (status & GELIC_CARD_PORT_STATUS_CHANGED) {
gelic_card_get_ether_port_status(card, 1);
+ }
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#620: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1273:
+static int gelic_ether_set_link_ksettings(struct net_device *netdev,
+ const struct ethtool_link_ksettings *cmd)
WARNING:BRACES: braces {} are not necessary for single statement blocks
#637: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1309:
+ if (ret) {
return ret;
+ }
WARNING:BRACES: braces {} are not necessary for any arm of this statement
#649: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1319:
+ if (ps3_compare_firmware_version(2, 2, 0) >= 0) {
[...]
- else
[...]
WARNING:BRACES: braces {} are not necessary for single statement blocks
#676: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1342:
+ if (wol->wolopts & ~WAKE_MAGIC) {
return -EINVAL;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#750: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1422:
+ if (!(netdev->flags & IFF_UP)) {
goto out;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#788: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1506:
+ if (GELIC_CARD_RX_CSUM_DEFAULT) {
netdev->features |= NETIF_F_RXCSUM;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#822: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1577:
+ if (!p) {
return NULL;
+ }
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#930: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1754:
+ result = request_irq(card->irq, gelic_card_interrupt, 0, netdev->name,
+ card);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#943: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1766:
+ result = gelic_card_init_chain(card, &card->tx_chain, card->descr,
+ GELIC_NET_TX_DESCRIPTORS);
WARNING:BRACES: braces {} are not necessary for single statement blocks
#948: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1768:
+ if (result) {
goto fail_alloc_tx;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#959: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1776:
+ if (result) {
goto fail_alloc_rx;
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#975: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1789:
+ if (result) {
goto fail_alloc_skbs;
+ }
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#1008: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1831:
+ lv1_net_set_interrupt_status_indicator(bus_id(card), bus_id(card), 0,
+ 0);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#1050: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1883:
+ lv1_net_set_interrupt_status_indicator(bus_id(card), dev_id(card), 0,
+ 0);
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Commit 518e332861e3 ("net/ps3_gelic: Format cleanups") has style problems, please review.
NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH
EMAIL_SUBJECT FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 379 ++++++++++---------
> 1 file changed, 193 insertions(+), 186 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index ba008a98928a..ded467d81f36 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -44,8 +44,6 @@ MODULE_AUTHOR("SCE Inc.");
> MODULE_DESCRIPTION("Gelic Network driver");
> MODULE_LICENSE("GPL");
>
> -
> -/* set irq_mask */
> int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
> {
> struct device *dev = ctodev(card);
> @@ -65,6 +63,7 @@ static void gelic_card_rx_irq_on(struct gelic_card *card)
> card->irq_mask |= GELIC_CARD_RXINT;
> gelic_card_set_irq_mask(card, card->irq_mask);
> }
> +
> static void gelic_card_rx_irq_off(struct gelic_card *card)
> {
> card->irq_mask &= ~GELIC_CARD_RXINT;
> @@ -72,15 +71,14 @@ static void gelic_card_rx_irq_off(struct gelic_card *card)
> }
>
> static void gelic_card_get_ether_port_status(struct gelic_card *card,
> - int inform)
> + int inform)
> {
> u64 v2;
> struct net_device *ether_netdev;
>
> lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_ETH_PORT_STATUS,
> - GELIC_LV1_VLAN_TX_ETHERNET_0, 0, 0,
> - &card->ether_port_status, &v2);
> + GELIC_LV1_GET_ETH_PORT_STATUS, GELIC_LV1_VLAN_TX_ETHERNET_0, 0,
> + 0, &card->ether_port_status, &v2);
>
> if (inform) {
> ether_netdev = card->netdev[GELIC_PORT_ETHERNET_0];
> @@ -100,7 +98,8 @@ static void gelic_card_get_ether_port_status(struct gelic_card *card,
> static enum gelic_descr_dma_status
> gelic_descr_get_status(struct gelic_descr *descr)
> {
> - return be32_to_cpu(descr->hw_regs.dmac_cmd_status) & GELIC_DESCR_DMA_STAT_MASK;
> + return be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> + GELIC_DESCR_DMA_STAT_MASK;
> }
>
> static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> @@ -110,8 +109,9 @@ static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> u64 v1, v2;
>
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_NEGOTIATION_MODE,
> - GELIC_LV1_PHY_ETHERNET_0, mode, 0, &v1, &v2);
> + GELIC_LV1_SET_NEGOTIATION_MODE, GELIC_LV1_PHY_ETHERNET_0, mode,
> + 0, &v1, &v2);
> +
> if (status) {
> dev_err(dev, "%s:%d: Failed setting negotiation mode: %d\n",
> __func__, __LINE__, status);
> @@ -138,7 +138,8 @@ static void gelic_card_disable_txdmac(struct gelic_card *card)
> status = lv1_net_stop_tx_dma(bus_id(card), dev_id(card));
>
> if (status) {
> - dev_err(dev, "lv1_net_stop_tx_dma failed, status=%d\n", status);
> + dev_err(dev, "%s:%d: lv1_net_stop_tx_dma failed: %d\n",
> + __func__, __LINE__, status);
> }
> }
>
> @@ -166,10 +167,11 @@ static void gelic_card_enable_rxdmac(struct gelic_card *card)
> }
> #endif
> status = lv1_net_start_rx_dma(bus_id(card), dev_id(card),
> - card->rx_chain.head->link.cpu_addr, 0);
> + card->rx_chain.head->link.cpu_addr, 0);
> +
> if (status) {
> - dev_err(dev, "lv1_net_start_rx_dma failed, status=%d\n",
> - status);
> + dev_err(dev, "%s:%d: lv1_net_start_rx_dma failed: %d\n",
> + __func__, __LINE__, status);
> }
> }
>
> @@ -189,7 +191,8 @@ static void gelic_card_disable_rxdmac(struct gelic_card *card)
> status = lv1_net_stop_rx_dma(bus_id(card), dev_id(card));
>
> if (status) {
> - dev_err(dev, "lv1_net_stop_rx_dma failed, %d\n", status);
> + dev_err(dev, "%s:%d: lv1_net_stop_rx_dma failed: %d\n",
> + __func__, __LINE__, status);
> }
> }
>
> @@ -202,11 +205,11 @@ static void gelic_card_disable_rxdmac(struct gelic_card *card)
> * in the status
> */
> static void gelic_descr_set_status(struct gelic_descr *descr,
> - enum gelic_descr_dma_status status)
> + enum gelic_descr_dma_status status)
> {
> descr->hw_regs.dmac_cmd_status = cpu_to_be32(status |
> - (be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> - ~GELIC_DESCR_DMA_STAT_MASK));
> + (be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> + ~GELIC_DESCR_DMA_STAT_MASK));
> /*
> * dma_cmd_status field is used to indicate whether the descriptor
> * is valid or not.
> @@ -226,14 +229,14 @@ static void gelic_descr_set_status(struct gelic_descr *descr,
> * and re-initialize the hardware chain for later use
> */
> static void gelic_card_reset_chain(struct gelic_card *card,
> - struct gelic_descr_chain *chain,
> - struct gelic_descr *start_descr)
> + struct gelic_descr_chain *chain, struct gelic_descr *start_descr)
> {
> struct gelic_descr *descr;
>
> for (descr = start_descr; start_descr != descr->next; descr++) {
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> - descr->hw_regs.next_descr_addr = cpu_to_be32(descr->next->link.cpu_addr);
> + descr->hw_regs.next_descr_addr =
> + cpu_to_be32(descr->next->link.cpu_addr);
> }
>
> chain->head = start_descr;
> @@ -249,11 +252,8 @@ void gelic_card_up(struct gelic_card *card)
> mutex_lock(&card->updown_lock);
> if (atomic_inc_return(&card->users) == 1) {
> dev_dbg(dev, "%s:%d: Starting...\n", __func__, __LINE__);
> - /* enable irq */
> gelic_card_set_irq_mask(card, card->irq_mask);
> - /* start rx */
> gelic_card_enable_rxdmac(card);
> -
> napi_enable(&card->napi);
> }
> mutex_unlock(&card->updown_lock);
> @@ -269,17 +269,14 @@ void gelic_card_down(struct gelic_card *card)
> dev_dbg(dev, "%s:%d: Stopping...\n", __func__, __LINE__);
> napi_disable(&card->napi);
> /*
> - * Disable irq. Wireless interrupts will
> - * be disabled later if any
> + * Disable irq. Wireless interrupts will be disabled later.
> */
> mask = card->irq_mask & (GELIC_CARD_WLAN_EVENT_RECEIVED |
> - GELIC_CARD_WLAN_COMMAND_COMPLETED);
> + GELIC_CARD_WLAN_COMMAND_COMPLETED);
> gelic_card_set_irq_mask(card, mask);
> - /* stop rx */
> gelic_card_disable_rxdmac(card);
> gelic_card_reset_chain(card, &card->rx_chain,
> - card->descr + GELIC_NET_TX_DESCRIPTORS);
> - /* stop tx */
> + card->descr + GELIC_NET_TX_DESCRIPTORS);
> gelic_card_disable_txdmac(card);
> }
> mutex_unlock(&card->updown_lock);
> @@ -291,12 +288,13 @@ void gelic_card_down(struct gelic_card *card)
> * @descr_in: address of desc
> */
> static void gelic_card_free_chain(struct gelic_card *card,
> - struct gelic_descr *descr_in)
> + struct gelic_descr *descr_in)
> {
> struct device *dev = ctodev(card);
> struct gelic_descr *descr;
>
> - for (descr = descr_in; descr && descr->link.cpu_addr; descr = descr->next) {
> + for (descr = descr_in; descr && descr->link.cpu_addr;
> + descr = descr->next) {
> dma_unmap_single(dev, descr->link.cpu_addr, descr->link.size,
> DMA_BIDIRECTIONAL);
> descr->link.cpu_addr = 0;
> @@ -316,8 +314,8 @@ static void gelic_card_free_chain(struct gelic_card *card,
> * returns 0 on success, <0 on failure
> */
> static int gelic_card_init_chain(struct gelic_card *card,
> - struct gelic_descr_chain *chain,
> - struct gelic_descr *start_descr, int no)
> + struct gelic_descr_chain *chain, struct gelic_descr *start_descr,
> + int no)
> {
> int i;
> struct gelic_descr *descr;
> @@ -326,7 +324,6 @@ static int gelic_card_init_chain(struct gelic_card *card,
> descr = start_descr;
> memset(descr, 0, sizeof(*descr) * no);
>
> - /* set up the hardware pointers in each descriptor */
> for (i = 0; i < no; i++, descr++) {
> descr->link.size = sizeof(struct gelic_hw_regs);
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> @@ -340,14 +337,14 @@ static int gelic_card_init_chain(struct gelic_card *card,
> descr->next = descr + 1;
> descr->prev = descr - 1;
> }
> - /* make them as ring */
> +
> (descr - 1)->next = start_descr;
> start_descr->prev = (descr - 1);
>
> - /* chain bus addr of hw descriptor */
> descr = start_descr;
> for (i = 0; i < no; i++, descr++) {
> - descr->hw_regs.next_descr_addr = cpu_to_be32(descr->next->link.cpu_addr);
> + descr->hw_regs.next_descr_addr =
> + cpu_to_be32(descr->next->link.cpu_addr);
> }
>
> chain->head = start_descr;
> @@ -378,7 +375,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
> * Activate the descriptor state-wise
> */
> static int gelic_descr_prepare_rx(struct gelic_card *card,
> - struct gelic_descr *descr)
> + struct gelic_descr *descr)
> {
> struct device *dev = ctodev(card);
> int offset;
> @@ -439,14 +436,13 @@ static void gelic_card_release_rx_chain(struct gelic_card *card)
> do {
> if (descr->skb) {
> dma_unmap_single(dev,
> - be32_to_cpu(descr->hw_regs.payload.dev_addr),
> - descr->skb->len,
> - DMA_FROM_DEVICE);
> + be32_to_cpu(descr->hw_regs.payload.dev_addr),
> + descr->skb->len, DMA_FROM_DEVICE);
> descr->hw_regs.payload.dev_addr = 0;
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> gelic_descr_set_status(descr,
> - GELIC_DESCR_DMA_NOT_IN_USE);
> + GELIC_DESCR_DMA_NOT_IN_USE);
> }
> descr = descr->next;
> } while (descr != card->rx_chain.head);
> @@ -504,15 +500,16 @@ static int gelic_card_alloc_rx_skbs(struct gelic_card *card)
> * releases a used tx descriptor (unmapping, freeing of skb)
> */
> static void gelic_descr_release_tx(struct gelic_card *card,
> - struct gelic_descr *descr)
> + struct gelic_descr *descr)
> {
> struct sk_buff *skb = descr->skb;
> struct device *dev = ctodev(card);
>
> - BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status) & GELIC_DESCR_TX_TAIL));
> + BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status) &
> + GELIC_DESCR_TX_TAIL));
>
> - dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr), skb->len,
> - DMA_TO_DEVICE);
> + dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> + skb->len, DMA_TO_DEVICE);
> dev_kfree_skb_any(skb);
>
> descr->hw_regs.payload.dev_addr = 0;
> @@ -524,7 +521,6 @@ static void gelic_descr_release_tx(struct gelic_card *card,
> descr->hw_regs.data_error = 0;
> descr->skb = NULL;
>
> - /* set descr status */
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> }
>
> @@ -580,19 +576,19 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
> }
> break;
>
> - case GELIC_DESCR_DMA_CARDOWNED:
> - /* pending tx request */
> default:
> - /* any other value (== GELIC_DESCR_DMA_NOT_IN_USE) */
> - if (!stop)
> + if (!stop) {
> goto out;
> + }
> }
> +
> gelic_descr_release_tx(card, tx_chain->tail);
> - release ++;
> + release++;
> }
> out:
> - if (!stop && release)
> + if (!stop && release) {
> gelic_card_wake_queues(card);
> + }
> }
>
> /**
> @@ -613,18 +609,18 @@ void gelic_net_set_multi(struct net_device *netdev)
> u64 addr;
> int status;
>
> - /* clear all multicast address */
> status = lv1_net_remove_multicast_address(bus_id(card), dev_id(card),
> - 0, 1);
> + 0, 1);
> +
> if (status) {
> dev_err(dev,
> "%s:%d: lv1_net_remove_multicast_address failed %d\n",
> __func__, __LINE__, status);
> }
>
> - /* set broadcast address */
> status = lv1_net_add_multicast_address(bus_id(card), dev_id(card),
> - GELIC_NET_BROADCAST_ADDR, 0);
> + GELIC_NET_BROADCAST_ADDR, 0);
> +
> if (status) {
> dev_err(dev,
> "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> @@ -634,8 +630,8 @@ void gelic_net_set_multi(struct net_device *netdev)
> if ((netdev->flags & IFF_ALLMULTI) ||
> (netdev_mc_count(netdev) > GELIC_NET_MC_COUNT_MAX)) {
> status = lv1_net_add_multicast_address(bus_id(card),
> - dev_id(card),
> - 0, 1);
> + dev_id(card), 0, 1);
> +
> if (status) {
> dev_err(dev,
> "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> @@ -644,7 +640,6 @@ void gelic_net_set_multi(struct net_device *netdev)
> return;
> }
>
> - /* set multicast addresses */
> netdev_for_each_mc_addr(ha, netdev) {
> addr = 0;
> p = ha->addr;
> @@ -653,8 +648,8 @@ void gelic_net_set_multi(struct net_device *netdev)
> addr |= *p++;
> }
> status = lv1_net_add_multicast_address(bus_id(card),
> - dev_id(card),
> - addr, 0);
> + dev_id(card), addr, 0);
> +
> if (status) {
> dev_err(dev,
> "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> @@ -698,8 +693,8 @@ gelic_card_get_next_tx_descr(struct gelic_card *card)
> return NULL;
> /* see if the next descriptor is free */
> if (card->tx_chain.tail != card->tx_chain.head->next &&
> - gelic_descr_get_status(card->tx_chain.head) ==
> - GELIC_DESCR_DMA_NOT_IN_USE)
> + gelic_descr_get_status(card->tx_chain.head) ==
> + GELIC_DESCR_DMA_NOT_IN_USE)
> return card->tx_chain.head;
> else
> return NULL;
> @@ -716,12 +711,12 @@ gelic_card_get_next_tx_descr(struct gelic_card *card)
> * has executed before.
> */
> static void gelic_descr_set_tx_cmdstat(struct gelic_descr *descr,
> - struct sk_buff *skb)
> + struct sk_buff *skb)
> {
> if (skb->ip_summed != CHECKSUM_PARTIAL)
> descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_NO_CHKSUM |
> - GELIC_DESCR_TX_DMA_FRAME_TAIL);
> + GELIC_DESCR_TX_DMA_FRAME_TAIL);
> else {
> /* is packet ip?
> * if yes: tcp? udp? */
> @@ -747,14 +742,15 @@ static void gelic_descr_set_tx_cmdstat(struct gelic_descr *descr,
> }
>
> static struct sk_buff *gelic_put_vlan_tag(struct sk_buff *skb,
> - unsigned short tag)
> + unsigned short tag)
> {
> struct vlan_ethhdr *veth;
> static unsigned int c;
>
> if (skb_headroom(skb) < VLAN_HLEN) {
> struct sk_buff *sk_tmp = skb;
> - pr_debug("%s: hd=%d c=%ud\n", __func__, skb_headroom(skb), c);
> + pr_debug("%s:%d: hd=%d c=%ud\n", __func__, __LINE__,
> + skb_headroom(skb), c);
> skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
> if (!skb)
> return NULL;
> @@ -781,8 +777,7 @@ static struct sk_buff *gelic_put_vlan_tag(struct sk_buff *skb,
> *
> */
> static int gelic_descr_prepare_tx(struct gelic_card *card,
> - struct gelic_descr *descr,
> - struct sk_buff *skb)
> + struct gelic_descr *descr, struct sk_buff *skb)
> {
> struct device *dev = ctodev(card);
> dma_addr_t cpu_addr;
> @@ -792,10 +787,11 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
> enum gelic_port_type type;
>
> type = netdev_port(skb->dev)->type;
> - skb_tmp = gelic_put_vlan_tag(skb,
> - card->vlan[type].tx);
> - if (!skb_tmp)
> + skb_tmp = gelic_put_vlan_tag(skb, card->vlan[type].tx);
> +
> + if (!skb_tmp) {
> return -ENOMEM;
> + }
> skb = skb_tmp;
> }
>
> @@ -890,7 +886,8 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
> * link this prepared descriptor to previous one
> * to achieve high performance
> */
> - descr->prev->hw_regs.next_descr_addr = cpu_to_be32(descr->link.cpu_addr);
> + descr->prev->hw_regs.next_descr_addr =
> + cpu_to_be32(descr->link.cpu_addr);
> /*
> * as hardware descriptor is modified in the above lines,
> * ensure that the hardware sees it
> @@ -926,9 +923,7 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
> * stack. The descriptor state is not changed.
> */
> static void gelic_net_pass_skb_up(struct gelic_descr *descr,
> - struct gelic_card *card,
> - struct net_device *netdev)
> -
> + struct gelic_card *card, struct net_device *netdev)
> {
> struct device *dev = ctodev(card);
> struct sk_buff *skb = descr->skb;
> @@ -938,19 +933,18 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
> data_error = be32_to_cpu(descr->hw_regs.data_error);
> /* unmap skb buffer */
> dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> - GELIC_NET_MAX_MTU,
> - DMA_FROM_DEVICE);
> + GELIC_NET_MAX_MTU, DMA_FROM_DEVICE);
>
> - skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size)?
> + skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size) ?
> be32_to_cpu(descr->hw_regs.valid_size) :
> be32_to_cpu(descr->hw_regs.result_size));
>
> if (!descr->hw_regs.valid_size) {
> dev_err(dev, "%s:%d: buffer full %x %x %x\n", __func__,
> __LINE__,
> - be32_to_cpu(descr->hw_regs.result_size),
> - be32_to_cpu(descr->hw_regs.payload.size),
> - be32_to_cpu(descr->hw_regs.dmac_cmd_status));
> + be32_to_cpu(descr->hw_regs.result_size),
> + be32_to_cpu(descr->hw_regs.payload.size),
> + be32_to_cpu(descr->hw_regs.dmac_cmd_status));
> }
>
> descr->skb = NULL;
> @@ -1028,8 +1022,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> netdev = card->netdev[GELIC_PORT_ETHERNET_0];
>
> if ((status == GELIC_DESCR_DMA_RESPONSE_ERROR) ||
> - (status == GELIC_DESCR_DMA_PROTECTION_ERROR) ||
> - (status == GELIC_DESCR_DMA_FORCE_END)) {
> + (status == GELIC_DESCR_DMA_PROTECTION_ERROR) ||
> + (status == GELIC_DESCR_DMA_FORCE_END)) {
> dev_info(dev, "%s:%d: dropping RX descriptor with state %x\n",
> __func__, __LINE__, status);
> netdev->stats.rx_dropped++;
> @@ -1064,8 +1058,7 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> refill:
>
> /* is the current descriptor terminated with next_descr == NULL? */
> - dmac_chain_ended =
> - be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> + dmac_chain_ended = be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> GELIC_DESCR_RX_DMA_CHAIN_END;
> /*
> * So that always DMAC can see the end
> @@ -1089,15 +1082,17 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> /*
> * Set this descriptor the end of the chain.
> */
> - descr->prev->hw_regs.next_descr_addr = cpu_to_be32(descr->link.cpu_addr);
> + descr->prev->hw_regs.next_descr_addr =
> + cpu_to_be32(descr->link.cpu_addr);
>
> /*
> * If dmac chain was met, DMAC stopped.
> * thus re-enable it
> */
>
> - if (dmac_chain_ended)
> + if (dmac_chain_ended) {
> gelic_card_enable_rxdmac(card);
> + }
>
> return 1;
> }
> @@ -1116,9 +1111,9 @@ static int gelic_net_poll(struct napi_struct *napi, int budget)
> int packets_done = 0;
>
> while (packets_done < budget) {
> - if (!gelic_card_decode_one_descr(card))
> + if (!gelic_card_decode_one_descr(card)) {
> break;
> -
> + }
> packets_done++;
> }
>
> @@ -1126,6 +1121,7 @@ static int gelic_net_poll(struct napi_struct *napi, int budget)
> napi_complete_done(napi, packets_done);
> gelic_card_rx_irq_on(card);
> }
> +
> return packets_done;
> }
>
> @@ -1140,8 +1136,9 @@ static irqreturn_t gelic_card_interrupt(int irq, void *ptr)
>
> status = card->irq_status;
>
> - if (!status)
> + if (!status) {
> return IRQ_NONE;
> + }
>
> status &= card->irq_mask;
>
> @@ -1160,13 +1157,15 @@ static irqreturn_t gelic_card_interrupt(int irq, void *ptr)
> }
>
> /* ether port status changed */
> - if (status & GELIC_CARD_PORT_STATUS_CHANGED)
> + if (status & GELIC_CARD_PORT_STATUS_CHANGED) {
> gelic_card_get_ether_port_status(card, 1);
> + }
>
> #ifdef CONFIG_GELIC_WIRELESS
> if (status & (GELIC_CARD_WLAN_EVENT_RECEIVED |
> - GELIC_CARD_WLAN_COMMAND_COMPLETED))
> + GELIC_CARD_WLAN_COMMAND_COMPLETED)) {
> gelic_wl_interrupt(card->netdev[GELIC_PORT_WIRELESS], status);
> + }
> #endif
>
> return IRQ_HANDLED;
> @@ -1211,14 +1210,14 @@ int gelic_net_open(struct net_device *netdev)
> }
>
> void gelic_net_get_drvinfo(struct net_device *netdev,
> - struct ethtool_drvinfo *info)
> + struct ethtool_drvinfo *info)
> {
> strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
> strlcpy(info->version, DRV_VERSION, sizeof(info->version));
> }
>
> static int gelic_ether_get_link_ksettings(struct net_device *netdev,
> - struct ethtool_link_ksettings *cmd)
> + struct ethtool_link_ksettings *cmd)
> {
> struct gelic_card *card = netdev_card(netdev);
> struct device *dev = ctodev(card);
> @@ -1248,10 +1247,12 @@ static int gelic_ether_get_link_ksettings(struct net_device *netdev,
> }
>
> supported = SUPPORTED_TP | SUPPORTED_Autoneg |
> - SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
> - SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
> - SUPPORTED_1000baseT_Full;
> + SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
> + SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
> + SUPPORTED_1000baseT_Full;
> +
> advertising = supported;
> +
> if (card->link_mode & GELIC_LV1_ETHER_AUTO_NEG) {
> cmd->base.autoneg = AUTONEG_ENABLE;
> } else {
> @@ -1261,16 +1262,15 @@ static int gelic_ether_get_link_ksettings(struct net_device *netdev,
> cmd->base.port = PORT_TP;
>
> ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
> - supported);
> + supported);
> ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.advertising,
> - advertising);
> + advertising);
>
> return 0;
> }
>
> -static int
> -gelic_ether_set_link_ksettings(struct net_device *netdev,
> - const struct ethtool_link_ksettings *cmd)
> +static int gelic_ether_set_link_ksettings(struct net_device *netdev,
> + const struct ethtool_link_ksettings *cmd)
> {
> struct gelic_card *card = netdev_card(netdev);
> struct device *dev = ctodev(card);
> @@ -1293,6 +1293,7 @@ gelic_ether_set_link_ksettings(struct net_device *netdev,
> default:
> return -EINVAL;
> }
> +
> if (cmd->base.duplex == DUPLEX_FULL) {
> mode |= GELIC_LV1_ETHER_FULL_DUPLEX;
> } else if (cmd->base.speed == SPEED_1000) {
> @@ -1305,25 +1306,28 @@ gelic_ether_set_link_ksettings(struct net_device *netdev,
>
> ret = gelic_card_set_link_mode(card, mode);
>
> - if (ret)
> + if (ret) {
> return ret;
> + }
>
> return 0;
> }
>
> static void gelic_net_get_wol(struct net_device *netdev,
> - struct ethtool_wolinfo *wol)
> + struct ethtool_wolinfo *wol)
> {
> - if (0 <= ps3_compare_firmware_version(2, 2, 0))
> + if (ps3_compare_firmware_version(2, 2, 0) >= 0) {
> wol->supported = WAKE_MAGIC;
> - else
> + } else {
> wol->supported = 0;
> + }
>
> wol->wolopts = ps3_sys_manager_get_wol() ? wol->supported : 0;
> memset(&wol->sopass, 0, sizeof(wol->sopass));
> }
> +
> static int gelic_net_set_wol(struct net_device *netdev,
> - struct ethtool_wolinfo *wol)
> + struct ethtool_wolinfo *wol)
> {
> struct gelic_card *card = netdev_card(netdev);
> struct device *dev = ctodev(card);
> @@ -1331,56 +1335,56 @@ static int gelic_net_set_wol(struct net_device *netdev,
> u64 v1, v2;
>
> if (ps3_compare_firmware_version(2, 2, 0) < 0 ||
> - !capable(CAP_NET_ADMIN))
> + !capable(CAP_NET_ADMIN)) {
> return -EPERM;
> + }
>
> - if (wol->wolopts & ~WAKE_MAGIC)
> + if (wol->wolopts & ~WAKE_MAGIC) {
> return -EINVAL;
> + }
>
> if (wol->wolopts & WAKE_MAGIC) {
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_MAGIC_PACKET,
> - 0, GELIC_LV1_WOL_MP_ENABLE,
> - &v1, &v2);
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_MAGIC_PACKET, 0,
> + GELIC_LV1_WOL_MP_ENABLE, &v1, &v2);
> +
> if (status) {
> dev_dbg(dev, "%s:%d: Enabling WOL failed: %d\n",
> __func__, __LINE__, status);
> status = -EIO;
> goto done;
> }
> +
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_ADD_MATCH_ADDR,
> - 0, GELIC_LV1_WOL_MATCH_ALL,
> - &v1, &v2);
> - if (!status)
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_ADD_MATCH_ADDR, 0,
> + GELIC_LV1_WOL_MATCH_ALL, &v1, &v2);
> +
> + if (!status) {
> ps3_sys_manager_set_wol(1);
> - else {
> + } else {
> dev_dbg(dev, "%s:%d: Enabling WOL filter failed: %d\n",
> __func__, __LINE__, status);
> status = -EIO;
> }
> } else {
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_MAGIC_PACKET,
> - 0, GELIC_LV1_WOL_MP_DISABLE,
> - &v1, &v2);
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_MAGIC_PACKET,
> + 0, GELIC_LV1_WOL_MP_DISABLE, &v1, &v2);
> +
> if (status) {
> dev_dbg(dev, "%s:%d: Disabling WOL failed: %d\n",
> __func__, __LINE__, status);
> status = -EIO;
> goto done;
> }
> +
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_DELETE_MATCH_ADDR,
> - 0, GELIC_LV1_WOL_MATCH_ALL,
> - &v1, &v2);
> - if (!status)
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_DELETE_MATCH_ADDR,
> + 0, GELIC_LV1_WOL_MATCH_ALL, &v1, &v2);
> +
> + if (!status) {
> ps3_sys_manager_set_wol(0);
> - else {
> + } else {
> dev_dbg(dev, "%s:%d: Removing WOL filter failed: %d\n",
> __func__, __LINE__, status);
> status = -EIO;
> @@ -1415,8 +1419,9 @@ static void gelic_net_tx_timeout_task(struct work_struct *work)
>
> dev_info(dev, "%s:%d: Timed out. Restarting...\n", __func__, __LINE__);
>
> - if (!(netdev->flags & IFF_UP))
> + if (!(netdev->flags & IFF_UP)) {
> goto out;
> + }
>
> netif_device_detach(netdev);
> gelic_net_stop(netdev);
> @@ -1441,10 +1446,12 @@ void gelic_net_tx_timeout(struct net_device *netdev, unsigned int txqueue)
>
> card = netdev_card(netdev);
> atomic_inc(&card->tx_timeout_task_counter);
> - if (netdev->flags & IFF_UP)
> +
> + if (netdev->flags & IFF_UP) {
> schedule_work(&card->tx_timeout_task);
> - else
> + } else {
> atomic_dec(&card->tx_timeout_task_counter);
> + }
> }
>
> static const struct net_device_ops gelic_netdevice_ops = {
> @@ -1468,7 +1475,7 @@ static const struct net_device_ops gelic_netdevice_ops = {
> * fills out function pointers in the net_device structure
> */
> static void gelic_ether_setup_netdev_ops(struct net_device *netdev,
> - struct napi_struct *napi)
> + struct napi_struct *napi)
> {
> netdev->watchdog_timeo = GELIC_NET_WATCHDOG_TIMEOUT;
> /* NAPI */
> @@ -1494,20 +1501,23 @@ int gelic_net_setup_netdev(struct net_device *netdev, struct gelic_card *card)
> u64 v1, v2;
>
> netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
> -
> netdev->features = NETIF_F_IP_CSUM;
> - if (GELIC_CARD_RX_CSUM_DEFAULT)
> +
> + if (GELIC_CARD_RX_CSUM_DEFAULT) {
> netdev->features |= NETIF_F_RXCSUM;
> + }
>
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_MAC_ADDRESS,
> - 0, 0, 0, &v1, &v2);
> + GELIC_LV1_GET_MAC_ADDRESS, 0, 0, 0, &v1, &v2);
> +
> v1 <<= 16;
> +
> if (status || !is_valid_ether_addr((u8 *)&v1)) {
> dev_dbg(dev, "%s:%d: lv1_net_control GET_MAC_ADDR failed: %d\n",
> __func__, __LINE__, status);
> return -EINVAL;
> }
> +
> memcpy(netdev->dev_addr, &v1, ETH_ALEN);
>
> if (card->vlan_required) {
> @@ -1557,34 +1567,32 @@ static struct gelic_card *gelic_alloc_card_net(struct net_device **netdev)
> */
> BUILD_BUG_ON(offsetof(struct gelic_card, irq_status) % 8);
> BUILD_BUG_ON(offsetof(struct gelic_card, descr) % 32);
> - alloc_size =
> - sizeof(struct gelic_card) +
> + alloc_size = sizeof(struct gelic_card) +
> sizeof(struct gelic_descr) * GELIC_NET_RX_DESCRIPTORS +
> sizeof(struct gelic_descr) * GELIC_NET_TX_DESCRIPTORS +
> GELIC_ALIGN - 1;
>
> p = kzalloc(alloc_size, GFP_KERNEL);
> - if (!p)
> +
> + if (!p) {
> return NULL;
> + }
> +
> card = PTR_ALIGN(p, GELIC_ALIGN);
> card->unalign = p;
>
> - /*
> - * alloc netdev
> - */
> *netdev = alloc_etherdev(sizeof(struct gelic_port));
> +
> if (!*netdev) {
> kfree(card->unalign);
> return NULL;
> }
> port = netdev_priv(*netdev);
>
> - /* gelic_port */
> port->netdev = *netdev;
> port->card = card;
> port->type = GELIC_PORT_ETHERNET_0;
>
> - /* gelic_card */
> card->netdev[GELIC_PORT_ETHERNET_0] = *netdev;
>
> INIT_WORK(&card->tx_timeout_task, gelic_net_tx_timeout_task);
> @@ -1619,9 +1627,9 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
> for (i = 0; i < ARRAY_SIZE(vlan_id_ix); i++) {
> /* tx tag */
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_VLAN_ID,
> - vlan_id_ix[i].tx,
> - 0, 0, &v1, &v2);
> + GELIC_LV1_GET_VLAN_ID, vlan_id_ix[i].tx, 0, 0, &v1,
> + &v2);
> +
> if (status || !v1) {
> if (status != LV1_NO_ENTRY) {
> dev_dbg(dev,
> @@ -1637,9 +1645,9 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
>
> /* rx tag */
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_VLAN_ID,
> - vlan_id_ix[i].rx,
> - 0, 0, &v1, &v2);
> + GELIC_LV1_GET_VLAN_ID, vlan_id_ix[i].rx, 0, 0, &v1,
> + &v2);
> +
> if (status || !v1) {
> if (status != LV1_NO_ENTRY) {
> dev_dbg(dev,
> @@ -1651,6 +1659,7 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
> card->vlan[i].rx = 0;
> continue;
> }
> +
> card->vlan[i].rx = (u16)v1;
>
> dev_dbg(dev, "%s:%d: vlan_id[%d] tx=%02x rx=%02x\n", __func__,
> @@ -1672,6 +1681,7 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
> dev_dbg(dev, "%s:%d: internal vlan %s\n", __func__, __LINE__,
> card->vlan_required ? "enabled" : "disabled");
> }
> +
> /*
> * ps3_gelic_driver_probe - add a device to the control of this driver
> */
> @@ -1703,27 +1713,24 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> goto fail_dma_region;
> }
>
> - /* alloc card/netdevice */
> card = gelic_alloc_card_net(&netdev);
> +
> if (!card) {
> dev_info(dev, "%s:%d: gelic_net_alloc_card failed.\n", __func__,
> __LINE__);
> result = -ENOMEM;
> goto fail_alloc_card;
> }
> +
> ps3_system_bus_set_drvdata(sb_dev, card);
> card->dev = sb_dev;
>
> - /* get internal vlan info */
> gelic_card_get_vlan_info(card);
>
> card->link_mode = GELIC_LV1_ETHER_AUTO_NEG;
>
> - /* setup interrupt */
> result = lv1_net_set_interrupt_status_indicator(bus_id(card),
> - dev_id(card),
> - ps3_mm_phys_to_lpar(__pa(&card->irq_status)),
> - 0);
> + dev_id(card), ps3_mm_phys_to_lpar(__pa(&card->irq_status)), 0);
>
> if (result) {
> dev_dbg(dev,
> @@ -1742,8 +1749,9 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> result = -EPERM;
> goto fail_alloc_irq;
> }
> - result = request_irq(card->irq, gelic_card_interrupt,
> - 0, netdev->name, card);
> +
> + result = request_irq(card->irq, gelic_card_interrupt, 0, netdev->name,
> + card);
>
> if (result) {
> dev_dbg(dev, "%s:%d: request_irq failed: %d\n",
> @@ -1751,22 +1759,24 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> goto fail_request_irq;
> }
>
> - /* setup card structure */
> card->irq_mask = GELIC_CARD_RXINT | GELIC_CARD_TXINT |
> GELIC_CARD_PORT_STATUS_CHANGED;
>
> + result = gelic_card_init_chain(card, &card->tx_chain, card->descr,
> + GELIC_NET_TX_DESCRIPTORS);
>
> - result = gelic_card_init_chain(card, &card->tx_chain,
> - card->descr, GELIC_NET_TX_DESCRIPTORS);
> - if (result)
> + if (result) {
> goto fail_alloc_tx;
> + }
> +
> result = gelic_card_init_chain(card, &card->rx_chain,
> - card->descr + GELIC_NET_TX_DESCRIPTORS,
> - GELIC_NET_RX_DESCRIPTORS);
> - if (result)
> + card->descr + GELIC_NET_TX_DESCRIPTORS,
> + GELIC_NET_RX_DESCRIPTORS);
> +
> + if (result) {
> goto fail_alloc_rx;
> + }
>
> - /* head of chain */
> card->tx_top = card->tx_chain.head;
> card->rx_top = card->rx_chain.head;
>
> @@ -1774,19 +1784,21 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> __func__, __LINE__, card->rx_top, card->tx_top,
> sizeof(struct gelic_descr), GELIC_NET_RX_DESCRIPTORS);
>
> - /* allocate rx skbs */
> result = gelic_card_alloc_rx_skbs(card);
> - if (result)
> +
> + if (result) {
> goto fail_alloc_skbs;
> + }
>
> spin_lock_init(&card->tx_lock);
> card->tx_dma_progress = 0;
>
> - /* setup net_device structure */
> netdev->irq = card->irq;
> SET_NETDEV_DEV(netdev, dev);
> gelic_ether_setup_netdev_ops(netdev, &card->napi);
> +
> result = gelic_net_setup_netdev(netdev, card);
> +
> if (result) {
> dev_err(dev, "%s:%d: setup_netdev failed: %d\n", __func__,
> __LINE__, result);
> @@ -1795,6 +1807,7 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
>
> #ifdef CONFIG_GELIC_WIRELESS
> result = gelic_wl_driver_probe(card);
> +
> if (result) {
> dev_dbg(dev, "%s:%d: WL init failed\n", __func__, __LINE__);
> goto fail_setup_netdev;
> @@ -1814,9 +1827,8 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> fail_request_irq:
> ps3_sb_event_receive_port_destroy(sb_dev, card->irq);
> fail_alloc_irq:
> - lv1_net_set_interrupt_status_indicator(bus_id(card),
> - bus_id(card),
> - 0, 0);
> + lv1_net_set_interrupt_status_indicator(bus_id(card), bus_id(card), 0,
> + 0);
> fail_status_indicator:
> ps3_system_bus_set_drvdata(sb_dev, NULL);
> kfree(netdev_card(netdev)->unalign);
> @@ -1842,20 +1854,16 @@ static void ps3_gelic_driver_remove(struct ps3_system_bus_device *sb_dev)
>
> dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
>
> - /* set auto-negotiation */
> gelic_card_set_link_mode(card, GELIC_LV1_ETHER_AUTO_NEG);
>
> #ifdef CONFIG_GELIC_WIRELESS
> gelic_wl_driver_remove(card);
> #endif
> - /* stop interrupt */
> gelic_card_set_irq_mask(card, 0);
>
> - /* turn off DMA, force end */
> gelic_card_disable_rxdmac(card);
> gelic_card_disable_txdmac(card);
>
> - /* release chains */
> gelic_card_release_tx_chain(card, 1);
> gelic_card_release_rx_chain(card);
>
> @@ -1863,16 +1871,16 @@ static void ps3_gelic_driver_remove(struct ps3_system_bus_device *sb_dev)
> gelic_card_free_chain(card, card->rx_top);
>
> netdev0 = card->netdev[GELIC_PORT_ETHERNET_0];
> - /* disconnect event port */
> +
> free_irq(card->irq, card);
> netdev0->irq = 0;
> ps3_sb_event_receive_port_destroy(card->dev, card->irq);
>
> wait_event(card->waitq,
> - atomic_read(&card->tx_timeout_task_counter) == 0);
> + atomic_read(&card->tx_timeout_task_counter) == 0);
>
> - lv1_net_set_interrupt_status_indicator(bus_id(card), dev_id(card),
> - 0 , 0);
> + lv1_net_set_interrupt_status_indicator(bus_id(card), dev_id(card), 0,
> + 0);
>
> unregister_netdev(netdev0);
> kfree(netdev_card(netdev0)->unalign);
> @@ -1896,14 +1904,14 @@ static struct ps3_system_bus_driver ps3_gelic_driver = {
> .core.owner = THIS_MODULE,
> };
>
> -static int __init ps3_gelic_driver_init (void)
> +static int __init ps3_gelic_driver_init(void)
> {
> return firmware_has_feature(FW_FEATURE_PS3_LV1)
> ? ps3_system_bus_driver_register(&ps3_gelic_driver)
> : -ENODEV;
> }
>
> -static void __exit ps3_gelic_driver_exit (void)
> +static void __exit ps3_gelic_driver_exit(void)
> {
> ps3_system_bus_driver_unregister(&ps3_gelic_driver);
> }
> @@ -1912,4 +1920,3 @@ module_init(ps3_gelic_driver_init);
> module_exit(ps3_gelic_driver_exit);
>
> MODULE_ALIAS(PS3_MODULE_ALIAS_GELIC);
> -
>
^ permalink raw reply
* Re: [PATCH v4 02/10] net/ps3_gelic: Use local dev variable
From: Christophe Leroy @ 2021-08-05 5:05 UTC (permalink / raw)
To: Geoff Levand, David S. Miller, Jakub Kicinski; +Cc: netdev, linuxppc-dev
In-Reply-To: <26f56a1c8332d227156cd33586b176a329570117.1627068552.git.geoff@infradead.org>
Le 23/07/2021 à 22:31, Geoff Levand a écrit :
> In an effort to make the PS3 gelic driver easier to maintain, add a
> local variable dev to those routines that use the device structure that
> makes the use the device structure more consistent.
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
Running checkpatch script provides the following feedback:
WARNING:BRACES: braces {} are not necessary for single statement blocks
#31: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:56:
+ if (status) {
+ dev_err(dev, "%s:%d failed: %d\n", __func__, __LINE__, status);
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#70: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:140:
+ if (status) {
+ dev_err(dev, "lv1_net_stop_tx_dma failed, status=%d\n", status);
+ }
WARNING:BRACES: braces {} are not necessary for single statement blocks
#111: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:191:
+ if (status) {
+ dev_err(dev, "lv1_net_stop_rx_dma failed, %d\n", status);
+ }
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#170: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:301:
+ dma_unmap_single(dev, descr->link.cpu_addr, descr->link.size,
+ DMA_BIDIRECTIONAL);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#190: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:335:
+ dma_map_single(dev, descr, descr->link.size,
+ DMA_BIDIRECTIONAL);
WARNING:BRACES: braces {} are not necessary for single statement blocks
#213: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:387:
+ if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) {
+ dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
+ }
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#226: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:413:
+ descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(dev,
descr->skb->data,
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#281: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:570:
+ dev_info_ratelimited(dev,
+ "%s:%d: forcing end of tx descriptor with status %x\n",
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#414: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:843:
+ dev_info(dev, "%s:%d: lv1_net_start_txdma failed: %d\n",
+ __func__, __LINE__, status);
WARNING:VSPRINTF_SPECIFIER_PX: Using vsprintf specifier '%px' potentially exposes the kernel memory
layout, if you don't really need the address please consider using '%p'.
#480: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1006:
+ dev_dbg(dev, "%s:%d: dormant descr? %px\n", __func__, __LINE__,
+ descr);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#491: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1024:
+ dev_info(dev, "%s:%d: unknown packet vid=%x\n",
+ __func__, __LINE__, vid);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#502: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1034:
+ dev_info(dev, "%s:%d: dropping RX descriptor with state %x\n",
+ __func__, __LINE__, status);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#687: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1535:
+ dev_info(dev, "%s:%d: %s MAC addr %pxM\n", __func__, __LINE__,
+ netdev->name, netdev->dev_addr);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#797: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1710:
+ dev_info(dev, "%s:%d: gelic_net_alloc_card failed.\n", __func__,
+ __LINE__);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#824: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1737:
+ result = ps3_sb_event_receive_port_setup(sb_dev, PS3_BINDING_CPU_ANY,
&card->irq);
WARNING:VSPRINTF_SPECIFIER_PX: Using vsprintf specifier '%px' potentially exposes the kernel memory
layout, if you don't really need the address please consider using '%p'.
#854: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:1773:
+ dev_dbg(dev, "%s:%d: descr rx %px, tx %px, size %#lx, num %#x\n",
+ __func__, __LINE__, card->rx_top, card->tx_top,
+ sizeof(struct gelic_descr), GELIC_NET_RX_DESCRIPTORS);
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Commit 244665a969da ("net/ps3_gelic: Use local dev variable") has style problems, please review.
NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH
EMAIL_SUBJECT FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS
Christophe
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 340 +++++++++++--------
> 1 file changed, 191 insertions(+), 149 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cb45571573d7..ba008a98928a 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -48,13 +48,15 @@ MODULE_LICENSE("GPL");
> /* set irq_mask */
> int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
> {
> + struct device *dev = ctodev(card);
> int status;
>
> status = lv1_net_set_interrupt_mask(bus_id(card), dev_id(card),
> mask, 0);
> - if (status)
> - dev_info(ctodev(card),
> - "%s failed %d\n", __func__, status);
> + if (status) {
> + dev_err(dev, "%s:%d failed: %d\n", __func__, __LINE__, status);
> + }
> +
> return status;
> }
>
> @@ -103,6 +105,7 @@ gelic_descr_get_status(struct gelic_descr *descr)
>
> static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> {
> + struct device *dev = ctodev(card);
> int status;
> u64 v1, v2;
>
> @@ -110,8 +113,8 @@ static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> GELIC_LV1_SET_NEGOTIATION_MODE,
> GELIC_LV1_PHY_ETHERNET_0, mode, 0, &v1, &v2);
> if (status) {
> - pr_info("%s: failed setting negotiation mode %d\n", __func__,
> - status);
> + dev_err(dev, "%s:%d: Failed setting negotiation mode: %d\n",
> + __func__, __LINE__, status);
> return -EBUSY;
> }
>
> @@ -128,13 +131,15 @@ static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> */
> static void gelic_card_disable_txdmac(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
>
> /* this hvc blocks until the DMA in progress really stopped */
> status = lv1_net_stop_tx_dma(bus_id(card), dev_id(card));
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_stop_tx_dma failed, status=%d\n", status);
> +
> + if (status) {
> + dev_err(dev, "lv1_net_stop_tx_dma failed, status=%d\n", status);
> + }
> }
>
> /**
> @@ -146,6 +151,7 @@ static void gelic_card_disable_txdmac(struct gelic_card *card)
> */
> static void gelic_card_enable_rxdmac(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
>
> #ifdef DEBUG
> @@ -161,9 +167,10 @@ static void gelic_card_enable_rxdmac(struct gelic_card *card)
> #endif
> status = lv1_net_start_rx_dma(bus_id(card), dev_id(card),
> card->rx_chain.head->link.cpu_addr, 0);
> - if (status)
> - dev_info(ctodev(card),
> - "lv1_net_start_rx_dma failed, status=%d\n", status);
> + if (status) {
> + dev_err(dev, "lv1_net_start_rx_dma failed, status=%d\n",
> + status);
> + }
> }
>
> /**
> @@ -175,13 +182,15 @@ static void gelic_card_enable_rxdmac(struct gelic_card *card)
> */
> static void gelic_card_disable_rxdmac(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
>
> /* this hvc blocks until the DMA in progress really stopped */
> status = lv1_net_stop_rx_dma(bus_id(card), dev_id(card));
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_stop_rx_dma failed, %d\n", status);
> +
> + if (status) {
> + dev_err(dev, "lv1_net_stop_rx_dma failed, %d\n", status);
> + }
> }
>
> /**
> @@ -235,10 +244,11 @@ static void gelic_card_reset_chain(struct gelic_card *card,
>
> void gelic_card_up(struct gelic_card *card)
> {
> - pr_debug("%s: called\n", __func__);
> + struct device *dev = ctodev(card);
> +
> mutex_lock(&card->updown_lock);
> if (atomic_inc_return(&card->users) == 1) {
> - pr_debug("%s: real do\n", __func__);
> + dev_dbg(dev, "%s:%d: Starting...\n", __func__, __LINE__);
> /* enable irq */
> gelic_card_set_irq_mask(card, card->irq_mask);
> /* start rx */
> @@ -247,16 +257,16 @@ void gelic_card_up(struct gelic_card *card)
> napi_enable(&card->napi);
> }
> mutex_unlock(&card->updown_lock);
> - pr_debug("%s: done\n", __func__);
> }
>
> void gelic_card_down(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> u64 mask;
> - pr_debug("%s: called\n", __func__);
> +
> mutex_lock(&card->updown_lock);
> if (atomic_dec_if_positive(&card->users) == 0) {
> - pr_debug("%s: real do\n", __func__);
> + dev_dbg(dev, "%s:%d: Stopping...\n", __func__, __LINE__);
> napi_disable(&card->napi);
> /*
> * Disable irq. Wireless interrupts will
> @@ -273,7 +283,6 @@ void gelic_card_down(struct gelic_card *card)
> gelic_card_disable_txdmac(card);
> }
> mutex_unlock(&card->updown_lock);
> - pr_debug("%s: done\n", __func__);
> }
>
> /**
> @@ -284,11 +293,12 @@ void gelic_card_down(struct gelic_card *card)
> static void gelic_card_free_chain(struct gelic_card *card,
> struct gelic_descr *descr_in)
> {
> + struct device *dev = ctodev(card);
> struct gelic_descr *descr;
>
> for (descr = descr_in; descr && descr->link.cpu_addr; descr = descr->next) {
> - dma_unmap_single(ctodev(card), descr->link.cpu_addr,
> - descr->link.size, DMA_BIDIRECTIONAL);
> + dma_unmap_single(dev, descr->link.cpu_addr, descr->link.size,
> + DMA_BIDIRECTIONAL);
> descr->link.cpu_addr = 0;
> }
> }
> @@ -311,6 +321,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
> {
> int i;
> struct gelic_descr *descr;
> + struct device *dev = ctodev(card);
>
> descr = start_descr;
> memset(descr, 0, sizeof(*descr) * no);
> @@ -320,9 +331,8 @@ static int gelic_card_init_chain(struct gelic_card *card,
> descr->link.size = sizeof(struct gelic_hw_regs);
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> descr->link.cpu_addr =
> - dma_map_single(ctodev(card), descr,
> - descr->link.size,
> - DMA_BIDIRECTIONAL);
> + dma_map_single(dev, descr, descr->link.size,
> + DMA_BIDIRECTIONAL);
>
> if (!descr->link.cpu_addr)
> goto iommu_error;
> @@ -351,7 +361,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
> iommu_error:
> for (i--, descr--; 0 <= i; i--, descr--)
> if (descr->link.cpu_addr)
> - dma_unmap_single(ctodev(card), descr->link.cpu_addr,
> + dma_unmap_single(dev, descr->link.cpu_addr,
> descr->link.size,
> DMA_BIDIRECTIONAL);
> return -ENOMEM;
> @@ -370,11 +380,14 @@ static int gelic_card_init_chain(struct gelic_card *card,
> static int gelic_descr_prepare_rx(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> + struct device *dev = ctodev(card);
> int offset;
> unsigned int bufsize;
>
> - if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
> - dev_info(ctodev(card), "%s: ERROR status\n", __func__);
> + if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) {
> + dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
> + }
> +
> /* we need to round up the buffer size to a multiple of 128 */
> bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
>
> @@ -396,14 +409,14 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
> if (offset)
> skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> /* io-mmu-map the skb */
> - descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(ctodev(card),
> + descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(dev,
> descr->skb->data,
> GELIC_NET_MAX_MTU,
> DMA_FROM_DEVICE));
> if (!descr->hw_regs.payload.dev_addr) {
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> - dev_info(ctodev(card),
> + dev_info(dev,
> "%s:Could not iommu-map rx buffer\n", __func__);
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> return -ENOMEM;
> @@ -421,10 +434,11 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
> static void gelic_card_release_rx_chain(struct gelic_card *card)
> {
> struct gelic_descr *descr = card->rx_chain.head;
> + struct device *dev = ctodev(card);
>
> do {
> if (descr->skb) {
> - dma_unmap_single(ctodev(card),
> + dma_unmap_single(dev,
> be32_to_cpu(descr->hw_regs.payload.dev_addr),
> descr->skb->len,
> DMA_FROM_DEVICE);
> @@ -493,10 +507,11 @@ static void gelic_descr_release_tx(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> struct sk_buff *skb = descr->skb;
> + struct device *dev = ctodev(card);
>
> BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status) & GELIC_DESCR_TX_TAIL));
>
> - dma_unmap_single(ctodev(card), be32_to_cpu(descr->hw_regs.payload.dev_addr), skb->len,
> + dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr), skb->len,
> DMA_TO_DEVICE);
> dev_kfree_skb_any(skb);
>
> @@ -538,6 +553,7 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
> {
> struct gelic_descr_chain *tx_chain;
> enum gelic_descr_dma_status status;
> + struct device *dev = ctodev(card);
> struct net_device *netdev;
> int release = 0;
>
> @@ -550,11 +566,9 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
> case GELIC_DESCR_DMA_RESPONSE_ERROR:
> case GELIC_DESCR_DMA_PROTECTION_ERROR:
> case GELIC_DESCR_DMA_FORCE_END:
> - if (printk_ratelimit())
> - dev_info(ctodev(card),
> - "%s: forcing end of tx descriptor " \
> - "with status %x\n",
> - __func__, status);
> + dev_info_ratelimited(dev,
> + "%s:%d: forcing end of tx descriptor with status %x\n",
> + __func__, __LINE__, status);
> netdev->stats.tx_dropped++;
> break;
>
> @@ -592,6 +606,7 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
> void gelic_net_set_multi(struct net_device *netdev)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> struct netdev_hw_addr *ha;
> unsigned int i;
> uint8_t *p;
> @@ -601,27 +616,31 @@ void gelic_net_set_multi(struct net_device *netdev)
> /* clear all multicast address */
> status = lv1_net_remove_multicast_address(bus_id(card), dev_id(card),
> 0, 1);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_remove_multicast_address failed %d\n",
> - status);
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_remove_multicast_address failed %d\n",
> + __func__, __LINE__, status);
> + }
> +
> /* set broadcast address */
> status = lv1_net_add_multicast_address(bus_id(card), dev_id(card),
> GELIC_NET_BROADCAST_ADDR, 0);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_add_multicast_address failed, %d\n",
> - status);
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> + __func__, __LINE__, status);
> + }
>
> if ((netdev->flags & IFF_ALLMULTI) ||
> (netdev_mc_count(netdev) > GELIC_NET_MC_COUNT_MAX)) {
> status = lv1_net_add_multicast_address(bus_id(card),
> dev_id(card),
> 0, 1);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_add_multicast_address failed, %d\n",
> - status);
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> + __func__, __LINE__, status);
> + }
> return;
> }
>
> @@ -636,10 +655,11 @@ void gelic_net_set_multi(struct net_device *netdev)
> status = lv1_net_add_multicast_address(bus_id(card),
> dev_id(card),
> addr, 0);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_add_multicast_address failed, %d\n",
> - status);
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> + __func__, __LINE__, status);
> + }
> }
> }
>
> @@ -651,17 +671,17 @@ void gelic_net_set_multi(struct net_device *netdev)
> */
> int gelic_net_stop(struct net_device *netdev)
> {
> - struct gelic_card *card;
> + struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
>
> - pr_debug("%s: start\n", __func__);
> + dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
>
> netif_stop_queue(netdev);
> netif_carrier_off(netdev);
>
> - card = netdev_card(netdev);
> gelic_card_down(card);
>
> - pr_debug("%s: done\n", __func__);
> + dev_dbg(dev, "%s:%d: <\n", __func__, __LINE__);
> return 0;
> }
>
> @@ -764,6 +784,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
> struct gelic_descr *descr,
> struct sk_buff *skb)
> {
> + struct device *dev = ctodev(card);
> dma_addr_t cpu_addr;
>
> if (card->vlan_required) {
> @@ -778,12 +799,10 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
> skb = skb_tmp;
> }
>
> - cpu_addr = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
> + cpu_addr = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
>
> if (!cpu_addr) {
> - dev_err(ctodev(card),
> - "dma map 2 failed (%p, %i). Dropping packet\n",
> - skb->data, skb->len);
> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> return -ENOMEM;
> }
>
> @@ -808,6 +827,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
> static int gelic_card_kick_txdma(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> + struct device *dev = ctodev(card);
> int status = 0;
>
> if (card->tx_dma_progress)
> @@ -819,8 +839,8 @@ static int gelic_card_kick_txdma(struct gelic_card *card,
> descr->link.cpu_addr, 0);
> if (status) {
> card->tx_dma_progress = 0;
> - dev_info(ctodev(card), "lv1_net_start_txdma failed," \
> - "status=%d\n", status);
> + dev_info(dev, "%s:%d: lv1_net_start_txdma failed: %d\n",
> + __func__, __LINE__, status);
> }
> }
> return status;
> @@ -836,6 +856,7 @@ static int gelic_card_kick_txdma(struct gelic_card *card,
> netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> struct gelic_descr *descr;
> int result;
> unsigned long flags;
> @@ -888,7 +909,7 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
> card->tx_chain.head = descr;
> /* reset hw termination */
> descr->prev->hw_regs.next_descr_addr = 0;
> - dev_info(ctodev(card), "%s: kick failure\n", __func__);
> + dev_info(dev, "%s:%d: kick failure\n", __func__, __LINE__);
> }
>
> spin_unlock_irqrestore(&card->tx_lock, flags);
> @@ -909,24 +930,28 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
> struct net_device *netdev)
>
> {
> + struct device *dev = ctodev(card);
> struct sk_buff *skb = descr->skb;
> u32 data_status, data_error;
>
> data_status = be32_to_cpu(descr->hw_regs.data_status);
> data_error = be32_to_cpu(descr->hw_regs.data_error);
> /* unmap skb buffer */
> - dma_unmap_single(ctodev(card), be32_to_cpu(descr->hw_regs.payload.dev_addr),
> + dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> GELIC_NET_MAX_MTU,
> DMA_FROM_DEVICE);
>
> skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size)?
> be32_to_cpu(descr->hw_regs.valid_size) :
> be32_to_cpu(descr->hw_regs.result_size));
> - if (!descr->hw_regs.valid_size)
> - dev_info(ctodev(card), "buffer full %x %x %x\n",
> +
> + if (!descr->hw_regs.valid_size) {
> + dev_err(dev, "%s:%d: buffer full %x %x %x\n", __func__,
> + __LINE__,
> be32_to_cpu(descr->hw_regs.result_size),
> be32_to_cpu(descr->hw_regs.payload.size),
> be32_to_cpu(descr->hw_regs.dmac_cmd_status));
> + }
>
> descr->skb = NULL;
> /*
> @@ -968,6 +993,7 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> enum gelic_descr_dma_status status;
> struct gelic_descr_chain *chain = &card->rx_chain;
> struct gelic_descr *descr = chain->head;
> + struct device *dev = ctodev(card);
> struct net_device *netdev = NULL;
> int dmac_chain_ended;
>
> @@ -977,7 +1003,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> return 0;
>
> if (status == GELIC_DESCR_DMA_NOT_IN_USE) {
> - dev_dbg(ctodev(card), "dormant descr? %p\n", descr);
> + dev_dbg(dev, "%s:%d: dormant descr? %px\n", __func__, __LINE__,
> + descr);
> return 0;
> }
>
> @@ -993,7 +1020,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> }
> }
> if (GELIC_PORT_MAX <= i) {
> - pr_info("%s: unknown packet vid=%x\n", __func__, vid);
> + dev_info(dev, "%s:%d: unknown packet vid=%x\n",
> + __func__, __LINE__, vid);
> goto refill;
> }
> } else
> @@ -1002,8 +1030,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> if ((status == GELIC_DESCR_DMA_RESPONSE_ERROR) ||
> (status == GELIC_DESCR_DMA_PROTECTION_ERROR) ||
> (status == GELIC_DESCR_DMA_FORCE_END)) {
> - dev_info(ctodev(card), "dropping RX descriptor with state %x\n",
> - status);
> + dev_info(dev, "%s:%d: dropping RX descriptor with state %x\n",
> + __func__, __LINE__, status);
> netdev->stats.rx_dropped++;
> goto refill;
> }
> @@ -1018,7 +1046,7 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> * Anyway this frame was longer than the MTU,
> * just drop it.
> */
> - dev_info(ctodev(card), "overlength frame\n");
> + dev_info(dev, "%s:%d: overlength frame\n", __func__, __LINE__);
> goto refill;
> }
> /*
> @@ -1026,8 +1054,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> * be treated as error.
> */
> if (status != GELIC_DESCR_DMA_FRAME_END) {
> - dev_dbg(ctodev(card), "RX descriptor with state %x\n",
> - status);
> + dev_dbg(dev, "%s:%d: RX descriptor with state %x\n", __func__,
> + __LINE__, status);
> goto refill;
> }
>
> @@ -1174,14 +1202,11 @@ int gelic_net_open(struct net_device *netdev)
> {
> struct gelic_card *card = netdev_card(netdev);
>
> - dev_dbg(ctodev(card), " -> %s %p\n", __func__, netdev);
> -
> gelic_card_up(card);
>
> netif_start_queue(netdev);
> gelic_card_get_ether_port_status(card, 1);
>
> - dev_dbg(ctodev(card), " <- %s\n", __func__);
> return 0;
> }
>
> @@ -1196,6 +1221,7 @@ static int gelic_ether_get_link_ksettings(struct net_device *netdev,
> struct ethtool_link_ksettings *cmd)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> u32 supported, advertising;
>
> gelic_card_get_ether_port_status(card, 0);
> @@ -1216,7 +1242,7 @@ static int gelic_ether_get_link_ksettings(struct net_device *netdev,
> cmd->base.speed = SPEED_1000;
> break;
> default:
> - pr_info("%s: speed unknown\n", __func__);
> + dev_dbg(dev, "%s:%d: speed unknown\n", __func__, __LINE__);
> cmd->base.speed = SPEED_10;
> break;
> }
> @@ -1247,6 +1273,7 @@ gelic_ether_set_link_ksettings(struct net_device *netdev,
> const struct ethtool_link_ksettings *cmd)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> u64 mode;
> int ret;
>
> @@ -1269,7 +1296,9 @@ gelic_ether_set_link_ksettings(struct net_device *netdev,
> if (cmd->base.duplex == DUPLEX_FULL) {
> mode |= GELIC_LV1_ETHER_FULL_DUPLEX;
> } else if (cmd->base.speed == SPEED_1000) {
> - pr_info("1000 half duplex is not supported.\n");
> + dev_dbg(dev,
> + "%s:%d: 1000 half duplex is not supported.\n",
> + __func__, __LINE__);
> return -EINVAL;
> }
> }
> @@ -1296,8 +1325,9 @@ static void gelic_net_get_wol(struct net_device *netdev,
> static int gelic_net_set_wol(struct net_device *netdev,
> struct ethtool_wolinfo *wol)
> {
> + struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> int status;
> - struct gelic_card *card;
> u64 v1, v2;
>
> if (ps3_compare_firmware_version(2, 2, 0) < 0 ||
> @@ -1307,7 +1337,6 @@ static int gelic_net_set_wol(struct net_device *netdev,
> if (wol->wolopts & ~WAKE_MAGIC)
> return -EINVAL;
>
> - card = netdev_card(netdev);
> if (wol->wolopts & WAKE_MAGIC) {
> status = lv1_net_control(bus_id(card), dev_id(card),
> GELIC_LV1_SET_WOL,
> @@ -1315,8 +1344,8 @@ static int gelic_net_set_wol(struct net_device *netdev,
> 0, GELIC_LV1_WOL_MP_ENABLE,
> &v1, &v2);
> if (status) {
> - pr_info("%s: enabling WOL failed %d\n", __func__,
> - status);
> + dev_dbg(dev, "%s:%d: Enabling WOL failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> goto done;
> }
> @@ -1328,8 +1357,8 @@ static int gelic_net_set_wol(struct net_device *netdev,
> if (!status)
> ps3_sys_manager_set_wol(1);
> else {
> - pr_info("%s: enabling WOL filter failed %d\n",
> - __func__, status);
> + dev_dbg(dev, "%s:%d: Enabling WOL filter failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> }
> } else {
> @@ -1339,8 +1368,8 @@ static int gelic_net_set_wol(struct net_device *netdev,
> 0, GELIC_LV1_WOL_MP_DISABLE,
> &v1, &v2);
> if (status) {
> - pr_info("%s: disabling WOL failed %d\n", __func__,
> - status);
> + dev_dbg(dev, "%s:%d: Disabling WOL failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> goto done;
> }
> @@ -1352,8 +1381,8 @@ static int gelic_net_set_wol(struct net_device *netdev,
> if (!status)
> ps3_sys_manager_set_wol(0);
> else {
> - pr_info("%s: removing WOL filter failed %d\n",
> - __func__, status);
> + dev_dbg(dev, "%s:%d: Removing WOL filter failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> }
> }
> @@ -1382,8 +1411,9 @@ static void gelic_net_tx_timeout_task(struct work_struct *work)
> struct gelic_card *card =
> container_of(work, struct gelic_card, tx_timeout_task);
> struct net_device *netdev = card->netdev[GELIC_PORT_ETHERNET_0];
> + struct device *dev = ctodev(card);
>
> - dev_info(ctodev(card), "%s:Timed out. Restarting...\n", __func__);
> + dev_info(dev, "%s:%d: Timed out. Restarting...\n", __func__, __LINE__);
>
> if (!(netdev->flags & IFF_UP))
> goto out;
> @@ -1459,6 +1489,7 @@ static void gelic_ether_setup_netdev_ops(struct net_device *netdev,
> **/
> int gelic_net_setup_netdev(struct net_device *netdev, struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
> u64 v1, v2;
>
> @@ -1473,9 +1504,8 @@ int gelic_net_setup_netdev(struct net_device *netdev, struct gelic_card *card)
> 0, 0, 0, &v1, &v2);
> v1 <<= 16;
> if (status || !is_valid_ether_addr((u8 *)&v1)) {
> - dev_info(ctodev(card),
> - "%s:lv1_net_control GET_MAC_ADDR failed %d\n",
> - __func__, status);
> + dev_dbg(dev, "%s:%d: lv1_net_control GET_MAC_ADDR failed: %d\n",
> + __func__, __LINE__, status);
> return -EINVAL;
> }
> memcpy(netdev->dev_addr, &v1, ETH_ALEN);
> @@ -1494,13 +1524,15 @@ int gelic_net_setup_netdev(struct net_device *netdev, struct gelic_card *card)
> netdev->max_mtu = GELIC_NET_MAX_MTU;
>
> status = register_netdev(netdev);
> +
> if (status) {
> - dev_err(ctodev(card), "%s:Couldn't register %s %d\n",
> - __func__, netdev->name, status);
> + dev_err(dev, "%s:%d: Couldn't register %s: %d\n", __func__,
> + __LINE__, netdev->name, status);
> return status;
> }
> - dev_info(ctodev(card), "%s: MAC addr %pM\n",
> - netdev->name, netdev->dev_addr);
> +
> + dev_info(dev, "%s:%d: %s MAC addr %pxM\n", __func__, __LINE__,
> + netdev->name, netdev->dev_addr);
>
> return 0;
> }
> @@ -1566,6 +1598,7 @@ static struct gelic_card *gelic_alloc_card_net(struct net_device **netdev)
>
> static void gelic_card_get_vlan_info(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> u64 v1, v2;
> int status;
> unsigned int i;
> @@ -1590,10 +1623,12 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
> vlan_id_ix[i].tx,
> 0, 0, &v1, &v2);
> if (status || !v1) {
> - if (status != LV1_NO_ENTRY)
> - dev_dbg(ctodev(card),
> - "get vlan id for tx(%d) failed(%d)\n",
> - vlan_id_ix[i].tx, status);
> + if (status != LV1_NO_ENTRY) {
> + dev_dbg(dev,
> + "%s:%d: Get vlan id for tx(%d) failed: %d\n",
> + __func__, __LINE__, vlan_id_ix[i].tx,
> + status);
> + }
> card->vlan[i].tx = 0;
> card->vlan[i].rx = 0;
> continue;
> @@ -1606,18 +1641,20 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
> vlan_id_ix[i].rx,
> 0, 0, &v1, &v2);
> if (status || !v1) {
> - if (status != LV1_NO_ENTRY)
> - dev_info(ctodev(card),
> - "get vlan id for rx(%d) failed(%d)\n",
> - vlan_id_ix[i].rx, status);
> + if (status != LV1_NO_ENTRY) {
> + dev_dbg(dev,
> + "%s:%d: Get vlan id for rx(%d) failed: %d\n",
> + __func__, __LINE__, vlan_id_ix[i].rx,
> + status);
> + }
> card->vlan[i].tx = 0;
> card->vlan[i].rx = 0;
> continue;
> }
> card->vlan[i].rx = (u16)v1;
>
> - dev_dbg(ctodev(card), "vlan_id[%d] tx=%02x rx=%02x\n",
> - i, card->vlan[i].tx, card->vlan[i].rx);
> + dev_dbg(dev, "%s:%d: vlan_id[%d] tx=%02x rx=%02x\n", __func__,
> + __LINE__, i, card->vlan[i].tx, card->vlan[i].rx);
> }
>
> if (card->vlan[GELIC_PORT_ETHERNET_0].tx) {
> @@ -1632,35 +1669,36 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
> card->vlan[GELIC_PORT_WIRELESS].rx = 0;
> }
>
> - dev_info(ctodev(card), "internal vlan %s\n",
> - card->vlan_required? "enabled" : "disabled");
> + dev_dbg(dev, "%s:%d: internal vlan %s\n", __func__, __LINE__,
> + card->vlan_required ? "enabled" : "disabled");
> }
> /*
> * ps3_gelic_driver_probe - add a device to the control of this driver
> */
> -static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> +static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> {
> + struct device *dev = &sb_dev->core;
> struct gelic_card *card;
> struct net_device *netdev;
> int result;
>
> - pr_debug("%s: called\n", __func__);
> + dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
>
> udbg_shutdown_ps3gelic();
>
> - result = ps3_open_hv_device(dev);
> + result = ps3_open_hv_device(sb_dev);
>
> if (result) {
> - dev_dbg(&dev->core, "%s:ps3_open_hv_device failed\n",
> - __func__);
> + dev_err(dev, "%s:%d: ps3_open_hv_device failed: %d\n",
> + __func__, __LINE__, result);
> goto fail_open;
> }
>
> - result = ps3_dma_region_create(dev->d_region);
> + result = ps3_dma_region_create(sb_dev->d_region);
>
> if (result) {
> - dev_dbg(&dev->core, "%s:ps3_dma_region_create failed(%d)\n",
> - __func__, result);
> + dev_err(dev, "%s:%d: ps3_dma_region_create failed: %d\n",
> + __func__, __LINE__, result);
> BUG_ON("check region type");
> goto fail_dma_region;
> }
> @@ -1668,13 +1706,13 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> /* alloc card/netdevice */
> card = gelic_alloc_card_net(&netdev);
> if (!card) {
> - dev_info(&dev->core, "%s:gelic_net_alloc_card failed\n",
> - __func__);
> + dev_info(dev, "%s:%d: gelic_net_alloc_card failed.\n", __func__,
> + __LINE__);
> result = -ENOMEM;
> goto fail_alloc_card;
> }
> - ps3_system_bus_set_drvdata(dev, card);
> - card->dev = dev;
> + ps3_system_bus_set_drvdata(sb_dev, card);
> + card->dev = sb_dev;
>
> /* get internal vlan info */
> gelic_card_get_vlan_info(card);
> @@ -1688,20 +1726,19 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> 0);
>
> if (result) {
> - dev_dbg(&dev->core,
> - "%s:set_interrupt_status_indicator failed: %s\n",
> - __func__, ps3_result(result));
> + dev_dbg(dev,
> + "%s:%d: set_interrupt_status_indicator failed: %s\n",
> + __func__, __LINE__, ps3_result(result));
> result = -EIO;
> goto fail_status_indicator;
> }
>
> - result = ps3_sb_event_receive_port_setup(dev, PS3_BINDING_CPU_ANY,
> + result = ps3_sb_event_receive_port_setup(sb_dev, PS3_BINDING_CPU_ANY,
> &card->irq);
>
> if (result) {
> - dev_info(ctodev(card),
> - "%s:gelic_net_open_device failed (%d)\n",
> - __func__, result);
> + dev_dbg(dev, "%s:%d: gelic_net_open_device failed: %d\n",
> + __func__, __LINE__, result);
> result = -EPERM;
> goto fail_alloc_irq;
> }
> @@ -1709,8 +1746,8 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> 0, netdev->name, card);
>
> if (result) {
> - dev_info(ctodev(card), "%s:request_irq failed (%d)\n",
> - __func__, result);
> + dev_dbg(dev, "%s:%d: request_irq failed: %d\n",
> + __func__, __LINE__, result);
> goto fail_request_irq;
> }
>
> @@ -1732,9 +1769,11 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> /* head of chain */
> card->tx_top = card->tx_chain.head;
> card->rx_top = card->rx_chain.head;
> - dev_dbg(ctodev(card), "descr rx %p, tx %p, size %#lx, num %#x\n",
> - card->rx_top, card->tx_top, sizeof(struct gelic_descr),
> - GELIC_NET_RX_DESCRIPTORS);
> +
> + dev_dbg(dev, "%s:%d: descr rx %px, tx %px, size %#lx, num %#x\n",
> + __func__, __LINE__, card->rx_top, card->tx_top,
> + sizeof(struct gelic_descr), GELIC_NET_RX_DESCRIPTORS);
> +
> /* allocate rx skbs */
> result = gelic_card_alloc_rx_skbs(card);
> if (result)
> @@ -1745,23 +1784,23 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
>
> /* setup net_device structure */
> netdev->irq = card->irq;
> - SET_NETDEV_DEV(netdev, &card->dev->core);
> + SET_NETDEV_DEV(netdev, dev);
> gelic_ether_setup_netdev_ops(netdev, &card->napi);
> result = gelic_net_setup_netdev(netdev, card);
> if (result) {
> - dev_dbg(&dev->core, "%s: setup_netdev failed %d\n",
> - __func__, result);
> + dev_err(dev, "%s:%d: setup_netdev failed: %d\n", __func__,
> + __LINE__, result);
> goto fail_setup_netdev;
> }
>
> #ifdef CONFIG_GELIC_WIRELESS
> result = gelic_wl_driver_probe(card);
> if (result) {
> - dev_dbg(&dev->core, "%s: WL init failed\n", __func__);
> + dev_dbg(dev, "%s:%d: WL init failed\n", __func__, __LINE__);
> goto fail_setup_netdev;
> }
> #endif
> - pr_debug("%s: done\n", __func__);
> + dev_dbg(dev, "%s:%d: < OK\n", __func__, __LINE__);
> return 0;
>
> fail_setup_netdev:
> @@ -1773,20 +1812,21 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> free_irq(card->irq, card);
> netdev->irq = 0;
> fail_request_irq:
> - ps3_sb_event_receive_port_destroy(dev, card->irq);
> + ps3_sb_event_receive_port_destroy(sb_dev, card->irq);
> fail_alloc_irq:
> lv1_net_set_interrupt_status_indicator(bus_id(card),
> bus_id(card),
> 0, 0);
> fail_status_indicator:
> - ps3_system_bus_set_drvdata(dev, NULL);
> + ps3_system_bus_set_drvdata(sb_dev, NULL);
> kfree(netdev_card(netdev)->unalign);
> free_netdev(netdev);
> fail_alloc_card:
> - ps3_dma_region_free(dev->d_region);
> + ps3_dma_region_free(sb_dev->d_region);
> fail_dma_region:
> - ps3_close_hv_device(dev);
> + ps3_close_hv_device(sb_dev);
> fail_open:
> + dev_dbg(dev, "%s:%d: < error\n", __func__, __LINE__);
> return result;
> }
>
> @@ -1794,11 +1834,13 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> * ps3_gelic_driver_remove - remove a device from the control of this driver
> */
>
> -static void ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
> +static void ps3_gelic_driver_remove(struct ps3_system_bus_device *sb_dev)
> {
> - struct gelic_card *card = ps3_system_bus_get_drvdata(dev);
> + struct gelic_card *card = ps3_system_bus_get_drvdata(sb_dev);
> + struct device *dev = &sb_dev->core;
> struct net_device *netdev0;
> - pr_debug("%s: called\n", __func__);
> +
> + dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
>
> /* set auto-negotiation */
> gelic_card_set_link_mode(card, GELIC_LV1_ETHER_AUTO_NEG);
> @@ -1836,13 +1878,13 @@ static void ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
> kfree(netdev_card(netdev0)->unalign);
> free_netdev(netdev0);
>
> - ps3_system_bus_set_drvdata(dev, NULL);
> + ps3_system_bus_set_drvdata(sb_dev, NULL);
>
> - ps3_dma_region_free(dev->d_region);
> + ps3_dma_region_free(sb_dev->d_region);
>
> - ps3_close_hv_device(dev);
> + ps3_close_hv_device(sb_dev);
>
> - pr_debug("%s: done\n", __func__);
> + dev_dbg(dev, "%s:%d: <\n", __func__, __LINE__);
> }
>
> static struct ps3_system_bus_driver ps3_gelic_driver = {
>
^ permalink raw reply
* Re: [PATCH v4 01/10] net/ps3_gelic: Add gelic_descr structures
From: Christophe Leroy @ 2021-08-05 5:03 UTC (permalink / raw)
To: Geoff Levand, David S. Miller, Jakub Kicinski; +Cc: netdev, linuxppc-dev
In-Reply-To: <c95aa8e57aca8b3af6893f13f2e03731f8198184.1627068552.git.geoff@infradead.org>
Le 23/07/2021 à 22:31, Geoff Levand a écrit :
> In an effort to make the PS3 gelic driver easier to maintain, create two
> new structures, struct gelic_hw_regs and struct gelic_chain_link, and
> replace the corresponding members of struct gelic_descr with the new
> structures.
>
> struct gelic_hw_regs holds the register variables used by the gelic
> hardware device. struct gelic_chain_link holds variables used to manage
> the driver's linked list of gelic descr structures.
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
Running checkpatch script provides the following feedback:
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#164: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:400:
+ descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(ctodev(card),
descr->skb->data,
WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG()
or BUG_ON()
#190: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:497:
+ BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status) & GELIC_DESCR_TX_TAIL));
ERROR:SPACING: spaces required around that '?' (ctx:VxE)
#333: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:922:
+ skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size)?
^
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Commit 5c33bdbe1861 ("net/ps3_gelic: Add gelic_descr structures") has style problems, please review.
NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH
EMAIL_SUBJECT FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 133 ++++++++++---------
> drivers/net/ethernet/toshiba/ps3_gelic_net.h | 24 ++--
> 2 files changed, 82 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 55e652624bd7..cb45571573d7 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -98,7 +98,7 @@ static void gelic_card_get_ether_port_status(struct gelic_card *card,
> static enum gelic_descr_dma_status
> gelic_descr_get_status(struct gelic_descr *descr)
> {
> - return be32_to_cpu(descr->dmac_cmd_status) & GELIC_DESCR_DMA_STAT_MASK;
> + return be32_to_cpu(descr->hw_regs.dmac_cmd_status) & GELIC_DESCR_DMA_STAT_MASK;
> }
>
> static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> @@ -154,13 +154,13 @@ static void gelic_card_enable_rxdmac(struct gelic_card *card)
> printk(KERN_ERR "%s: status=%x\n", __func__,
> be32_to_cpu(card->rx_chain.head->dmac_cmd_status));
> printk(KERN_ERR "%s: nextphy=%x\n", __func__,
> - be32_to_cpu(card->rx_chain.head->next_descr_addr));
> + be32_to_cpu(card->rx_chain.head->hw_regs.next_descr_addr));
> printk(KERN_ERR "%s: head=%p\n", __func__,
> card->rx_chain.head);
> }
> #endif
> status = lv1_net_start_rx_dma(bus_id(card), dev_id(card),
> - card->rx_chain.head->bus_addr, 0);
> + card->rx_chain.head->link.cpu_addr, 0);
> if (status)
> dev_info(ctodev(card),
> "lv1_net_start_rx_dma failed, status=%d\n", status);
> @@ -195,8 +195,8 @@ static void gelic_card_disable_rxdmac(struct gelic_card *card)
> static void gelic_descr_set_status(struct gelic_descr *descr,
> enum gelic_descr_dma_status status)
> {
> - descr->dmac_cmd_status = cpu_to_be32(status |
> - (be32_to_cpu(descr->dmac_cmd_status) &
> + descr->hw_regs.dmac_cmd_status = cpu_to_be32(status |
> + (be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> ~GELIC_DESCR_DMA_STAT_MASK));
> /*
> * dma_cmd_status field is used to indicate whether the descriptor
> @@ -224,13 +224,13 @@ static void gelic_card_reset_chain(struct gelic_card *card,
>
> for (descr = start_descr; start_descr != descr->next; descr++) {
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> - descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
> + descr->hw_regs.next_descr_addr = cpu_to_be32(descr->next->link.cpu_addr);
> }
>
> chain->head = start_descr;
> chain->tail = (descr - 1);
>
> - (descr - 1)->next_descr_addr = 0;
> + (descr - 1)->hw_regs.next_descr_addr = 0;
> }
>
> void gelic_card_up(struct gelic_card *card)
> @@ -286,10 +286,10 @@ static void gelic_card_free_chain(struct gelic_card *card,
> {
> struct gelic_descr *descr;
>
> - for (descr = descr_in; descr && descr->bus_addr; descr = descr->next) {
> - dma_unmap_single(ctodev(card), descr->bus_addr,
> - GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
> - descr->bus_addr = 0;
> + for (descr = descr_in; descr && descr->link.cpu_addr; descr = descr->next) {
> + dma_unmap_single(ctodev(card), descr->link.cpu_addr,
> + descr->link.size, DMA_BIDIRECTIONAL);
> + descr->link.cpu_addr = 0;
> }
> }
>
> @@ -317,13 +317,14 @@ static int gelic_card_init_chain(struct gelic_card *card,
>
> /* set up the hardware pointers in each descriptor */
> for (i = 0; i < no; i++, descr++) {
> + descr->link.size = sizeof(struct gelic_hw_regs);
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> - descr->bus_addr =
> + descr->link.cpu_addr =
> dma_map_single(ctodev(card), descr,
> - GELIC_DESCR_SIZE,
> + descr->link.size,
> DMA_BIDIRECTIONAL);
>
> - if (!descr->bus_addr)
> + if (!descr->link.cpu_addr)
> goto iommu_error;
>
> descr->next = descr + 1;
> @@ -336,22 +337,22 @@ static int gelic_card_init_chain(struct gelic_card *card,
> /* chain bus addr of hw descriptor */
> descr = start_descr;
> for (i = 0; i < no; i++, descr++) {
> - descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
> + descr->hw_regs.next_descr_addr = cpu_to_be32(descr->next->link.cpu_addr);
> }
>
> chain->head = start_descr;
> chain->tail = start_descr;
>
> /* do not chain last hw descriptor */
> - (descr - 1)->next_descr_addr = 0;
> + (descr - 1)->hw_regs.next_descr_addr = 0;
>
> return 0;
>
> iommu_error:
> for (i--, descr--; 0 <= i; i--, descr--)
> - if (descr->bus_addr)
> - dma_unmap_single(ctodev(card), descr->bus_addr,
> - GELIC_DESCR_SIZE,
> + if (descr->link.cpu_addr)
> + dma_unmap_single(ctodev(card), descr->link.cpu_addr,
> + descr->link.size,
> DMA_BIDIRECTIONAL);
> return -ENOMEM;
> }
> @@ -381,25 +382,25 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
> * bit more */
> descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
> if (!descr->skb) {
> - descr->buf_addr = 0; /* tell DMAC don't touch memory */
> + descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't touch memory */
> return -ENOMEM;
> }
> - descr->buf_size = cpu_to_be32(bufsize);
> - descr->dmac_cmd_status = 0;
> - descr->result_size = 0;
> - descr->valid_size = 0;
> - descr->data_error = 0;
> + descr->hw_regs.payload.size = cpu_to_be32(bufsize);
> + descr->hw_regs.dmac_cmd_status = 0;
> + descr->hw_regs.result_size = 0;
> + descr->hw_regs.valid_size = 0;
> + descr->hw_regs.data_error = 0;
>
> offset = ((unsigned long)descr->skb->data) &
> (GELIC_NET_RXBUF_ALIGN - 1);
> if (offset)
> skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> /* io-mmu-map the skb */
> - descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> + descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(ctodev(card),
> descr->skb->data,
> GELIC_NET_MAX_MTU,
> DMA_FROM_DEVICE));
> - if (!descr->buf_addr) {
> + if (!descr->hw_regs.payload.dev_addr) {
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> dev_info(ctodev(card),
> @@ -424,10 +425,10 @@ static void gelic_card_release_rx_chain(struct gelic_card *card)
> do {
> if (descr->skb) {
> dma_unmap_single(ctodev(card),
> - be32_to_cpu(descr->buf_addr),
> + be32_to_cpu(descr->hw_regs.payload.dev_addr),
> descr->skb->len,
> DMA_FROM_DEVICE);
> - descr->buf_addr = 0;
> + descr->hw_regs.payload.dev_addr = 0;
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> gelic_descr_set_status(descr,
> @@ -493,19 +494,19 @@ static void gelic_descr_release_tx(struct gelic_card *card,
> {
> struct sk_buff *skb = descr->skb;
>
> - BUG_ON(!(be32_to_cpu(descr->data_status) & GELIC_DESCR_TX_TAIL));
> + BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status) & GELIC_DESCR_TX_TAIL));
>
> - dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr), skb->len,
> + dma_unmap_single(ctodev(card), be32_to_cpu(descr->hw_regs.payload.dev_addr), skb->len,
> DMA_TO_DEVICE);
> dev_kfree_skb_any(skb);
>
> - descr->buf_addr = 0;
> - descr->buf_size = 0;
> - descr->next_descr_addr = 0;
> - descr->result_size = 0;
> - descr->valid_size = 0;
> - descr->data_status = 0;
> - descr->data_error = 0;
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> + descr->hw_regs.next_descr_addr = 0;
> + descr->hw_regs.result_size = 0;
> + descr->hw_regs.valid_size = 0;
> + descr->hw_regs.data_status = 0;
> + descr->hw_regs.data_error = 0;
> descr->skb = NULL;
>
> /* set descr status */
> @@ -698,7 +699,7 @@ static void gelic_descr_set_tx_cmdstat(struct gelic_descr *descr,
> struct sk_buff *skb)
> {
> if (skb->ip_summed != CHECKSUM_PARTIAL)
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_NO_CHKSUM |
> GELIC_DESCR_TX_DMA_FRAME_TAIL);
> else {
> @@ -706,19 +707,19 @@ static void gelic_descr_set_tx_cmdstat(struct gelic_descr *descr,
> * if yes: tcp? udp? */
> if (skb->protocol == htons(ETH_P_IP)) {
> if (ip_hdr(skb)->protocol == IPPROTO_TCP)
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_TCP_CHKSUM |
> GELIC_DESCR_TX_DMA_FRAME_TAIL);
>
> else if (ip_hdr(skb)->protocol == IPPROTO_UDP)
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_UDP_CHKSUM |
> GELIC_DESCR_TX_DMA_FRAME_TAIL);
> else /*
> * the stack should checksum non-tcp and non-udp
> * packets on his own: NETIF_F_IP_CSUM
> */
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_NO_CHKSUM |
> GELIC_DESCR_TX_DMA_FRAME_TAIL);
> }
> @@ -763,7 +764,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
> struct gelic_descr *descr,
> struct sk_buff *skb)
> {
> - dma_addr_t buf;
> + dma_addr_t cpu_addr;
>
> if (card->vlan_required) {
> struct sk_buff *skb_tmp;
> @@ -777,20 +778,20 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
> skb = skb_tmp;
> }
>
> - buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
> + cpu_addr = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
>
> - if (!buf) {
> + if (!cpu_addr) {
> dev_err(ctodev(card),
> "dma map 2 failed (%p, %i). Dropping packet\n",
> skb->data, skb->len);
> return -ENOMEM;
> }
>
> - descr->buf_addr = cpu_to_be32(buf);
> - descr->buf_size = cpu_to_be32(skb->len);
> + descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
> + descr->hw_regs.payload.size = cpu_to_be32(skb->len);
> descr->skb = skb;
> - descr->data_status = 0;
> - descr->next_descr_addr = 0; /* terminate hw descr */
> + descr->hw_regs.data_status = 0;
> + descr->hw_regs.next_descr_addr = 0; /* terminate hw descr */
> gelic_descr_set_tx_cmdstat(descr, skb);
>
> /* bump free descriptor pointer */
> @@ -815,7 +816,7 @@ static int gelic_card_kick_txdma(struct gelic_card *card,
> if (gelic_descr_get_status(descr) == GELIC_DESCR_DMA_CARDOWNED) {
> card->tx_dma_progress = 1;
> status = lv1_net_start_tx_dma(bus_id(card), dev_id(card),
> - descr->bus_addr, 0);
> + descr->link.cpu_addr, 0);
> if (status) {
> card->tx_dma_progress = 0;
> dev_info(ctodev(card), "lv1_net_start_txdma failed," \
> @@ -868,7 +869,7 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
> * link this prepared descriptor to previous one
> * to achieve high performance
> */
> - descr->prev->next_descr_addr = cpu_to_be32(descr->bus_addr);
> + descr->prev->hw_regs.next_descr_addr = cpu_to_be32(descr->link.cpu_addr);
> /*
> * as hardware descriptor is modified in the above lines,
> * ensure that the hardware sees it
> @@ -881,12 +882,12 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
> */
> netdev->stats.tx_dropped++;
> /* don't trigger BUG_ON() in gelic_descr_release_tx */
> - descr->data_status = cpu_to_be32(GELIC_DESCR_TX_TAIL);
> + descr->hw_regs.data_status = cpu_to_be32(GELIC_DESCR_TX_TAIL);
> gelic_descr_release_tx(card, descr);
> /* reset head */
> card->tx_chain.head = descr;
> /* reset hw termination */
> - descr->prev->next_descr_addr = 0;
> + descr->prev->hw_regs.next_descr_addr = 0;
> dev_info(ctodev(card), "%s: kick failure\n", __func__);
> }
>
> @@ -911,21 +912,21 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
> struct sk_buff *skb = descr->skb;
> u32 data_status, data_error;
>
> - data_status = be32_to_cpu(descr->data_status);
> - data_error = be32_to_cpu(descr->data_error);
> + data_status = be32_to_cpu(descr->hw_regs.data_status);
> + data_error = be32_to_cpu(descr->hw_regs.data_error);
> /* unmap skb buffer */
> - dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
> + dma_unmap_single(ctodev(card), be32_to_cpu(descr->hw_regs.payload.dev_addr),
> GELIC_NET_MAX_MTU,
> DMA_FROM_DEVICE);
>
> - skb_put(skb, be32_to_cpu(descr->valid_size)?
> - be32_to_cpu(descr->valid_size) :
> - be32_to_cpu(descr->result_size));
> - if (!descr->valid_size)
> + skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size)?
> + be32_to_cpu(descr->hw_regs.valid_size) :
> + be32_to_cpu(descr->hw_regs.result_size));
> + if (!descr->hw_regs.valid_size)
> dev_info(ctodev(card), "buffer full %x %x %x\n",
> - be32_to_cpu(descr->result_size),
> - be32_to_cpu(descr->buf_size),
> - be32_to_cpu(descr->dmac_cmd_status));
> + be32_to_cpu(descr->hw_regs.result_size),
> + be32_to_cpu(descr->hw_regs.payload.size),
> + be32_to_cpu(descr->hw_regs.dmac_cmd_status));
>
> descr->skb = NULL;
> /*
> @@ -1036,14 +1037,14 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
>
> /* is the current descriptor terminated with next_descr == NULL? */
> dmac_chain_ended =
> - be32_to_cpu(descr->dmac_cmd_status) &
> + be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> GELIC_DESCR_RX_DMA_CHAIN_END;
> /*
> * So that always DMAC can see the end
> * of the descriptor chain to avoid
> * from unwanted DMAC overrun.
> */
> - descr->next_descr_addr = 0;
> + descr->hw_regs.next_descr_addr = 0;
>
> /* change the descriptor state: */
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> @@ -1060,7 +1061,7 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
> /*
> * Set this descriptor the end of the chain.
> */
> - descr->prev->next_descr_addr = cpu_to_be32(descr->bus_addr);
> + descr->prev->hw_regs.next_descr_addr = cpu_to_be32(descr->link.cpu_addr);
>
> /*
> * If dmac chain was met, DMAC stopped.
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> index 68f324ed4eaf..569f691021d9 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> @@ -220,29 +220,35 @@ enum gelic_lv1_phy {
> GELIC_LV1_PHY_ETHERNET_0 = 0x0000000000000002L,
> };
>
> -/* size of hardware part of gelic descriptor */
> -#define GELIC_DESCR_SIZE (32)
> -
> enum gelic_port_type {
> GELIC_PORT_ETHERNET_0 = 0,
> GELIC_PORT_WIRELESS = 1,
> GELIC_PORT_MAX
> };
>
> -struct gelic_descr {
> - /* as defined by the hardware */
> - __be32 buf_addr;
> - __be32 buf_size;
> +/* as defined by the hardware */
> +struct gelic_hw_regs {
> + struct {
> + __be32 dev_addr;
> + __be32 size;
> + } __packed payload;
> __be32 next_descr_addr;
> __be32 dmac_cmd_status;
> __be32 result_size;
> __be32 valid_size; /* all zeroes for tx */
> __be32 data_status;
> __be32 data_error; /* all zeroes for tx */
> +} __packed;
>
> - /* used in the driver */
> +struct gelic_chain_link {
> + dma_addr_t cpu_addr;
> + unsigned int size;
> +};
> +
> +struct gelic_descr {
> + struct gelic_hw_regs hw_regs;
> + struct gelic_chain_link link;
> struct sk_buff *skb;
> - dma_addr_t bus_addr;
> struct gelic_descr *next;
> struct gelic_descr *prev;
> } __attribute__((aligned(32)));
>
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: John Ogness @ 2021-08-05 3:46 UTC (permalink / raw)
To: Daniel Thompson, Petr Mladek
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Bhaskar Chowdhury, x86, Ingo Molnar,
kgdb-bugreport, Nicholas Piggin, Borislav Petkov, Steven Rostedt,
Thomas Gleixner, Gustavo A. R. Silva, Sergey Senozhatsky,
Jason Wessel, linuxppc-dev, Cédric Le Goater
In-Reply-To: <20210804150431.qtra3wvh2n4m6j64@maple.lan>
On 2021-08-04, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On Wed, Aug 04, 2021 at 02:12:22PM +0200, Petr Mladek wrote:
>> On Wed 2021-08-04 12:31:59, Daniel Thompson wrote:
>> > On Tue, Aug 03, 2021 at 05:36:32PM +0206, John Ogness wrote:
>> > > On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> > > > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
>> > > >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
>> > > >> during cpu roundup. This will conflict with the printk cpulock.
>> > > >
>> > > > When the full vision is realized what will be the purpose of the printk
>> > > > cpulock?
>> > > >
>> > > > I'm asking largely because it's current role is actively unhelpful
>> > > > w.r.t. kdb. It is possible that cautious use of in_dbg_master() might
>> > > > be a better (and safer) solution. However it sounds like there is a
>> > > > larger role planned for the printk cpulock...
>> > >
>> > > The printk cpulock is used as a synchronization mechanism for
>> > > implementing atomic consoles, which need to be able to safely interrupt
>> > > the console write() activity at any time and immediately continue with
>> > > their own printing. The ultimate goal is to move all console printing
>> > > into per-console dedicated kthreads, so the primary function of the
>> > > printk cpulock is really to immediately _stop_ the CPU/kthread
>> > > performing write() in order to allow write_atomic() (from any context on
>> > > any CPU) to safely and reliably take over.
>> >
>> > I see.
>> >
>> > Is there any mileage in allowing in_dbg_master() to suppress taking
>> > the console lock?
>> >
>> > There's a couple of reasons to worry about the current approach.
>> >
>> > The first is that we don't want this code to trigger in the case when
>> > kgdb is enabled and kdb is not since it is only kdb (a self-hosted
>> > debugger) than uses the consoles. This case is relatively trivial to
>> > address since we can rename it kdb_roundup_delay() and alter the way it
>> > is conditionally compiled.
Well, _I_ want this code to trigger even without kdb. The printk cpulock
is meant to be the innermost locking for the entire kernel. No code is
allowed to block/spin on any kind of lock if holding the printk
cpulock. This is the only way to guarantee the functionality of the
atomic consoles.
For example, if the kernel were to crash while inside kgdb code, we want
to see the backtrace.
Since kgdb _does_ take locks (spinning on @dbg_slave_lock during roundup
and the master's own cpu lock as a retry loop on @dbg_master_lock),
clearly it is not allowed to hold the printk cpulock. The simplest
solution I could find was just to make sure kgdb_cpu_enter() isn't
called while holding the printk cpulock.
>> > The second is more of a problem however. kdb will only call into the
>> > console code from the debug master. By default this is the CPU that
>> > takes the debug trap so initial prints will work fine. However it is
>> > possible to switch to a different master (so we can read per-CPU
>> > registers and things like that). This will result in one of the CPUs
>> > that did the IPI round up calling into console code and this is unsafe
>> > in that instance.
It is only unsafe if a CPU enters "kgdb/kdb context" while holding the
printk cpulock. That is what I want to prevent.
>> > There are a couple of tricks we could adopt to work around this but
>> > given the slightly odd calling context for kdb (all CPUs quiesced, no
>> > log interleaving possible) it sounds like it would remain safe to
>> > bypass the lock if in_dbg_master() is true.
>> >
>> > Bypassing an inconvenient lock might sound icky but:
>> >
>> > 1. If the lock is not owned by any CPU then what kdb will do is safe.
No. The printk cpulock exists for low-level synchronization. The atomic
consoles need this synchronization. (For example, the 8250 needs this
for correct tracking of its interrupt register, even for
serial8250_put_poll_char().)
>> > 2. If the lock is owned by any CPU then we have quiesced it anyway
>> > and this makes is safe for the owning CPU to share its ownership
>> > (since it isn't much different to recursive acquisition on a single
>> > CPU)
Quiescing the printk cpulock is not permitted.
Just because it is kdb, does not mean that the atomic consoles were
interrupted in a convenient place. The whole purpose of the atomic
consoles is so that we can have guaranteed console output from _any_
context and _any_ line of code in the kernel.
>> I think about the following:
>>
>> void kgdb_roundup_cpus(void)
>> {
>> __printk_cpu_lock();
>> __kgdb_roundup_cpus();
>> }
>>
>> , where __printk_cpu_lock() waits/takes printk_cpu_lock()
>> __kgdb_roundup_cpus() is the original kgdb_roundup_cpus();
>>
>>
>> The idea is that kgdb_roundup_cpus() caller takes the printk_cpu lock.
>> The owner will be well defined.
>>
>> As a result any other CPU will not be able to take the printk_cpu lock
>> as long as it is owned by the kgdb lock. But as you say, kgdb will
>> make sure that everything is serialized at this stage. So that
>> the original raw_printk_cpu_lock_irqsave() might just disable
>> IRQs when called under debugger.
>>
>> Does it make any sense?
>
> Yes but I think it is still has problems.
>
> Primarily is doesn't solve the issue I raised. It would still be unsafe
> to change debug master: we can guarantee the initial master owns the
> lock but if it has been multiply acquired we cannot transfer ownership
> when we want to change master.
>
> Additionally it will delay the round up of cores that do not own the
> lock. The quiescing is never atomic and the operator needs to know
> that but the longer CPUs are allows to execute for the more confusing
> things can become for the operator.
>
> Finally on machines without an NMI this could cause trouble with the
> interrupt disable in raw_printk_cpu_lock_irqsave() (or any outer level
> interrupt disable). If the master get the lock then the other processes
> will become incapable of being rounded up if they are waiting for the
> printk lock).
I am also not happy with such a solution. Aside from Daniel's comments,
it also violates the basic principle of the printk cpulock by allowing
further locking while holding the print cpulock. That is a recipe for
deadlock.
>> I have to say that it is a bit hairy. But it looks slightly better
>> than the delayed/repeated IPI proposed by this patch.
>
> I'd like to reserve judgement for now which one is least worst...
> largely because if the purpose of the lock simply to prevent interleaving
> of console output then the debugger quiescing code should already have
> this covered.
>
> It leaves me wondering if a change like the one below is sufficient
> (based on code without John's patches but hopefully still clear enough).
> I've given the new code it's own branch which it doesn't, strictly
> speaking, need but it is easier to comment this way... and perhaps also
> just a little easier for people who have not set CONFIG_KGDB to
> ignore ;-).
>
> ~~~
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 142a58d124d9..41a7e103bb66 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3599,6 +3599,18 @@ int __printk_cpu_trylock(void)
> /* This CPU is already the owner. */
> atomic_inc(&printk_cpulock_nested);
> return 1;
> + } else if (in_dbg_master()) {
> + /*
> + * If we are executing as the primary CPU and with the debugger
> + * active than all other CPUs in the system are quiesced by
> + * the time kdb winds up calling this function. To execute this
> + * branch then the lock must be owned by one of the quiesced CPUs.
> + * Happily, because it is quiesced and cannot release it, it is
> + * safe for us to allow the lock to be taken from a different CPU!
> + * The lock will be released prior to resuming the real owner.
> + */
> + atomic_inc(&printk_cpulock_nested);
> + return 1;
> }
>
> return 0;
> ~~~
Being in kgdb/kdb context is similar to being in atomic console
context. (Of course, they are both using cpu locks.) But the contexts
are not the same. It is incorrect to handle them as the same.
We need to decide who is inside of who. Either printk is the innermost,
in which case the printk cpulock cannot be held when calling
kgdb_cpu_enter(). Or kgdb is the innermost, meaning that the atomic
consoles are no longer atomic/reliable while in kgdb.
I prefer and am pushing for the first, but am willing to accept the
second (i.e. that kgdb is the innermost function of the kernel).
> PS In the interested of full disclosure there is a special case
> in the debugger to allow it to try to cope if it fails to
> quiesce a CPU and I deliberately omitted this from the long
> comment above. That special case is expected to be unstable
> but since the alternative is likely to be a permanent deadlock
> without any indication of why we choose to take the risk of
> continuing. Personally I don't recommend reasoning about
> console safety based on this emergency case hence omitting the
> comment.
John Ogness
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Finn Thain @ 2021-08-05 1:54 UTC (permalink / raw)
To: Christophe Leroy; +Cc: userm57, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <b04a90a9-9d62-2192-f896-ea99be911604@csgroup.eu>
On Wed, 4 Aug 2021, Christophe Leroy wrote:
>
> This patch is related to the bisect you did that pointed to 4c0104a83fc3
> ("powerpc/32: Dismantle EXC_XFER_STD/LITE/TEMPLATE")
>
> I think maybe the starting point should be to (manually) apply the patch
> on top of that commit in order to check that the bug to leaded to
> pointing that commit as 'first bad commit' is now gone.
>
Stan has now confirmed this. He applied this patch on top of 4c0104a83fc3,
and it did indeed resolve the bug that 'git bisect' isolated [1]. Thanks
Christophe.
[1]
https://lore.kernel.org/lkml/666e3ab4-372-27c2-4621-7cc3933756dd@linux-m68k.org/
^ permalink raw reply
* [PATCH] powerpc/configs: Disable legacy ptys on microwatt defconfig
From: Anton Blanchard @ 2021-08-05 1:20 UTC (permalink / raw)
To: Paul Mackerras, Joel Stanley, Michael Ellerman; +Cc: linuxppc-dev
We shouldn't need legacy ptys, and disabling the option improves boot
time by about 0.5 seconds.
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
diff --git a/arch/powerpc/configs/microwatt_defconfig b/arch/powerpc/configs/microwatt_defconfig
index a08b739123da..ebc90aefbc0c 100644
--- a/arch/powerpc/configs/microwatt_defconfig
+++ b/arch/powerpc/configs/microwatt_defconfig
@@ -57,6 +57,7 @@ CONFIG_NETDEVICES=y
# CONFIG_INPUT is not set
# CONFIG_SERIO is not set
# CONFIG_VT is not set
+# CONFIG_LEGACY_PTYS is not set
CONFIG_SERIAL_8250=y
# CONFIG_SERIAL_8250_DEPRECATED_OPTIONS is not set
CONFIG_SERIAL_8250_CONSOLE=y
^ permalink raw reply related
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 19:55 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #7 from Elimar Riesebieter (riesebie@lxtec.de) ---
Created attachment 298205
--> https://bugzilla.kernel.org/attachment.cgi?id=298205&action=edit
dmesg dump with KALLSYMS enabled
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 19:54 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #6 from Elimar Riesebieter (riesebie@lxtec.de) ---
Created attachment 298203
--> https://bugzilla.kernel.org/attachment.cgi?id=298203&action=edit
config with KALLSYMS enabled
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 19:53 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #5 from Elimar Riesebieter (riesebie@lxtec.de) ---
Configuring KALLSYMS doesn't give the function names
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [PATCH] powerpc/smp: Fix OOPS in topology_init()
From: Christophe Leroy @ 2021-08-04 18:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Running an SMP kernel on an UP platform not prepared for it,
I encountered the following OOPS:
BUG: Kernel NULL pointer dereference on read at 0x00000034
Faulting instruction address: 0xc0a04110
Oops: Kernel access of bad area, sig: 11 [#1]
BE PAGE_SIZE=4K SMP NR_CPUS=2 CMPCPRO
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.13.0-pmac-00001-g230fedfaad21 #5234
NIP: c0a04110 LR: c0a040d8 CTR: c0a04084
REGS: e100dda0 TRAP: 0300 Not tainted (5.13.0-pmac-00001-g230fedfaad21)
MSR: 00009032 <EE,ME,IR,DR,RI> CR: 84000284 XER: 00000000
DAR: 00000034 DSISR: 20000000
GPR00: c0006bd4 e100de60 c1033320 00000000 00000000 c0942274 00000000 00000000
GPR08: 00000000 00000000 00000001 00000063 00000007 00000000 c0006f30 00000000
GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000005
GPR24: c0c67d74 c0c67f1c c0c60000 c0c67d70 c0c0c558 1efdf000 c0c00020 00000000
NIP [c0a04110] topology_init+0x8c/0x138
LR [c0a040d8] topology_init+0x54/0x138
Call Trace:
[e100de60] [80808080] 0x80808080 (unreliable)
[e100de90] [c0006bd4] do_one_initcall+0x48/0x1bc
[e100def0] [c0a0150c] kernel_init_freeable+0x1c8/0x278
[e100df20] [c0006f44] kernel_init+0x14/0x10c
[e100df30] [c00190fc] ret_from_kernel_thread+0x14/0x1c
Instruction dump:
7c692e70 7d290194 7c035040 7c7f1b78 5529103a 546706fe 5468103a 39400001
7c641b78 40800054 80c690b4 7fb9402e <81060034> 7fbeea14 2c080000 7fa3eb78
---[ end trace b246ffbc6bbbb6fb ]---
Fix it by checking smp_ops before using it, as already done in
several other places in the arch/powerpc/kernel/smp.c
Fixes: 39f87561454d ("powerpc/smp: Move ppc_md.cpu_die() to smp_ops.cpu_offline_self()")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 5ff0e55d0db1..defecb3b1b15 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -1167,7 +1167,7 @@ static int __init topology_init(void)
* CPU. For instance, the boot cpu might never be valid
* for hotplugging.
*/
- if (smp_ops->cpu_offline_self)
+ if (smp_ops && smp_ops->cpu_offline_self)
c->hotpluggable = 1;
#endif
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc: Always inline radix_enabled() to fix build failure
From: Erhard F. @ 2021-08-04 16:03 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev
In-Reply-To: <20210804013724.514468-1-jniethe5@gmail.com>
On Wed, 4 Aug 2021 11:37:24 +1000
Jordan Niethe <jniethe5@gmail.com> wrote:
> This is the same as commit acdad8fb4a15 ("powerpc: Force inlining of
> mmu_has_feature to fix build failure") but for radix_enabled(). The
> config in the linked bugzilla causes the following build failure:
> [...]
> The code relies on constant folding of MMU_FTRS_POSSIBLE at buildtime
> and elimination of non possible parts of code at compile time. For this
> to work radix_enabled() must be inlined so make it __always_inline.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=213803
> Reported-by: Erhard F. <erhard_f@mailbox.org>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/include/asm/mmu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index 27016b98ecb2..8abe8e42e045 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -324,7 +324,7 @@ static inline void assert_pte_locked(struct mm_struct *mm, unsigned long addr)
> }
> #endif /* !CONFIG_DEBUG_VM */
>
> -static inline bool radix_enabled(void)
> +static __always_inline bool radix_enabled(void)
> {
> return mmu_has_feature(MMU_FTR_TYPE_RADIX);
> }
> --
> 2.25.1
Thanks Jordan!
Your patch works well and my kernel build completes. Tested on v5.14-rc4. Only getting some warnings now:
[...]
CHK include/generated/autoksyms.h
GEN .version
CHK include/generated/compile.h
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms1.S
AS .tmp_vmlinux.kallsyms1.S
LD .tmp_vmlinux.kallsyms2
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms2.S
AS .tmp_vmlinux.kallsyms2.S
LD vmlinux
ld: warning: creating DT_TEXTREL in a PIE
SORTTAB vmlinux
SYSMAP System.map
CHKHEAD vmlinux
CHKREL vmlinux
GEN .version
CHK include/generated/compile.h
UPD include/generated/compile.h
CC init/version.o
AR init/built-in.a
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms1.S
AS .tmp_vmlinux.kallsyms1.S
LD .tmp_vmlinux.kallsyms2
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms2.S
AS .tmp_vmlinux.kallsyms2.S
LD vmlinux
ld: warning: creating DT_TEXTREL in a PIE
SORTTAB vmlinux
SYSMAP System.map
CHKHEAD vmlinux
CHKREL vmlinux
MODPOST modules-only.symvers
[...]
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Daniel Thompson @ 2021-08-04 15:04 UTC (permalink / raw)
To: Petr Mladek
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Bhaskar Chowdhury, x86, Ingo Molnar,
kgdb-bugreport, John Ogness, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <YQqEJtmNFxVxH3U/@alley>
On Wed, Aug 04, 2021 at 02:12:22PM +0200, Petr Mladek wrote:
> On Wed 2021-08-04 12:31:59, Daniel Thompson wrote:
> > On Tue, Aug 03, 2021 at 05:36:32PM +0206, John Ogness wrote:
> > > On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> > > >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> > > >> during cpu roundup. This will conflict with the printk cpulock.
> > > >
> > > > When the full vision is realized what will be the purpose of the printk
> > > > cpulock?
> > > >
> > > > I'm asking largely because it's current role is actively unhelpful
> > > > w.r.t. kdb. It is possible that cautious use of in_dbg_master() might
> > > > be a better (and safer) solution. However it sounds like there is a
> > > > larger role planned for the printk cpulock...
> > >
> > > The printk cpulock is used as a synchronization mechanism for
> > > implementing atomic consoles, which need to be able to safely interrupt
> > > the console write() activity at any time and immediately continue with
> > > their own printing. The ultimate goal is to move all console printing
> > > into per-console dedicated kthreads, so the primary function of the
> > > printk cpulock is really to immediately _stop_ the CPU/kthread
> > > performing write() in order to allow write_atomic() (from any context on
> > > any CPU) to safely and reliably take over.
> >
> > I see.
> >
> > Is there any mileage in allowing in_dbg_master() to suppress taking
> > the console lock?
> >
> > There's a couple of reasons to worry about the current approach.
> >
> > The first is that we don't want this code to trigger in the case when
> > kgdb is enabled and kdb is not since it is only kdb (a self-hosted
> > debugger) than uses the consoles. This case is relatively trivial to
> > address since we can rename it kdb_roundup_delay() and alter the way it
> > is conditionally compiled.
> >
> > The second is more of a problem however. kdb will only call into the
> > console code from the debug master. By default this is the CPU that
> > takes the debug trap so initial prints will work fine. However it is
> > possible to switch to a different master (so we can read per-CPU
> > registers and things like that). This will result in one of the CPUs
> > that did the IPI round up calling into console code and this is unsafe
> > in that instance.
> >
> > There are a couple of tricks we could adopt to work around this but
> > given the slightly odd calling context for kdb (all CPUs quiesced, no
> > log interleaving possible) it sounds like it would remain safe to
> > bypass the lock if in_dbg_master() is true.
> >
> > Bypassing an inconvenient lock might sound icky but:
> >
> > 1. If the lock is not owned by any CPU then what kdb will do is safe.
> >
> > 2. If the lock is owned by any CPU then we have quiesced it anyway
> > and this makes is safe for the owning CPU to share its ownership
> > (since it isn't much different to recursive acquisition on a single
> > CPU)
>
> I think about the following:
>
> void kgdb_roundup_cpus(void)
> {
> __printk_cpu_lock();
> __kgdb_roundup_cpus();
> }
>
> , where __printk_cpu_lock() waits/takes printk_cpu_lock()
> __kgdb_roundup_cpus() is the original kgdb_roundup_cpus();
>
>
> The idea is that kgdb_roundup_cpus() caller takes the printk_cpu lock.
> The owner will be well defined.
>
> As a result any other CPU will not be able to take the printk_cpu lock
> as long as it is owned by the kgdb lock. But as you say, kgdb will
> make sure that everything is serialized at this stage. So that
> the original raw_printk_cpu_lock_irqsave() might just disable
> IRQs when called under debugger.
>
> Does it make any sense?
Yes but I think it is still has problems.
Primarily is doesn't solve the issue I raised. It would still be unsafe
to change debug master: we can guarantee the initial master owns the
lock but if it has been multiply acquired we cannot transfer ownership
when we want to change master.
Additionally it will delay the round up of cores that do not own the
lock. The quiescing is never atomic and the operator needs to know
that but the longer CPUs are allows to execute for the more confusing
things can become for the operator.
Finally on machines without an NMI this could cause trouble with the
interrupt disable in raw_printk_cpu_lock_irqsave() (or any outer level
interrupt disable). If the master get the lock then the other processes
will become incapable of being rounded up if they are waiting for the
printk lock).
> I have to say that it is a bit hairy. But it looks slightly better
> than the delayed/repeated IPI proposed by this patch.
I'd like to reserve judgement for now which one is least worst...
largely because if the purpose of the lock simply to prevent interleaving
of console output then the debugger quiescing code should already have
this covered.
It leaves me wondering if a change like the one below is sufficient
(based on code without John's patches but hopefully still clear enough).
I've given the new code it's own branch which it doesn't, strictly
speaking, need but it is easier to comment this way... and perhaps also
just a little easier for people who have not set CONFIG_KGDB to
ignore ;-).
~~~
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 142a58d124d9..41a7e103bb66 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3599,6 +3599,18 @@ int __printk_cpu_trylock(void)
/* This CPU is already the owner. */
atomic_inc(&printk_cpulock_nested);
return 1;
+ } else if (in_dbg_master()) {
+ /*
+ * If we are executing as the primary CPU and with the debugger
+ * active than all other CPUs in the system are quiesced by
+ * the time kdb winds up calling this function. To execute this
+ * branch then the lock must be owned by one of the quiesced CPUs.
+ * Happily, because it is quiesced and cannot release it, it is
+ * safe for us to allow the lock to be taken from a different CPU!
+ * The lock will be released prior to resuming the real owner.
+ */
+ atomic_inc(&printk_cpulock_nested);
+ return 1;
}
return 0;
~~~
Daniel.
PS In the interested of full disclosure there is a special case
in the debugger to allow it to try to cope if it fails to
quiesce a CPU and I deliberately omitted this from the long
comment above. That special case is expected to be unstable
but since the alternative is likely to be a permanent deadlock
without any indication of why we choose to take the risk of
continuing. Personally I don't recommend reasoning about
console safety based on this emergency case hence omitting the
comment.
^ permalink raw reply related
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Nicholas Piggin @ 2021-08-04 14:45 UTC (permalink / raw)
To: Christophe Leroy
Cc: userm57, Finn Thain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <d8e1f924-ca60-4dcc-ac5f-3801ea226edf@csgroup.eu>
Excerpts from Christophe Leroy's message of August 4, 2021 11:28 pm:
>
>
> Le 04/08/2021 à 13:36, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of August 4, 2021 4:21 pm:
>>> Hi Nic,
>>>
>>> I think I'll need your help on that one.
>>>
>>> Le 04/08/2021 à 08:07, Christophe Leroy a écrit :
>>>>
>>>>
>>>> Le 04/08/2021 à 06:04, Finn Thain a écrit :
>>
>> Hi Finn!
>>
>>>>> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>>>>>
>>> ...
>>>>>
>>>>> ------------[ cut here ]------------
>>>>> kernel BUG at arch/powerpc/kernel/interrupt.c:49!
>>>>> Oops: Exception in kernel mode, sig: 5 [#1]
>>>>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
>>>>> Modules linked in:
>>>>> CPU: 0 PID: 1859 Comm: xfce4-session Not tainted 5.13.0-pmac-VMAP #10
>>>>> NIP: c0011474 LR: c0011464 CTR: 00000000
>>>>> REGS: e2f75e40 TRAP: 0700 Not tainted (5.13.0-pmac-VMAP)
>>>>> MSR: 00021032 <ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>>
>>>>> GPR00: c001604c e2f75f00 ca284a60 00000000 00000000 a5205eb0 00000008 00000020
>>>>> GPR08: ffffffc0 00000001 501200d9 ce030005 ca285010 00c1f778 00000000 00000000
>>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>>> GPR24: 00000000 ffffffc0 00000020 00000008 a5205eb0 00000000 e2f75f40 000000ae
>>>>> NIP [c0011474] system_call_exception+0x60/0x164
>>>>> LR [c0011464] system_call_exception+0x50/0x164
>>>>> Call Trace:
>>>>> [e2f75f00] [00009000] 0x9000 (unreliable)
>>>>> [e2f75f30] [c001604c] ret_from_syscall+0x0/0x28
>>>>> --- interrupt: c00 at 0xa69d6cb0
>>>>> NIP: a69d6cb0 LR: a69d6c3c CTR: 00000000
>>>>> REGS: e2f75f40 TRAP: 0c00 Not tainted (5.13.0-pmac-VMAP)
>>>>> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>>
>>>>> GPR00: 000000ae a5205de0 a5687ca0 00000000 00000000 a5205eb0 00000008 00000020
>>>>> GPR08: ffffffc0 401201ea 401200d9 ffffffff c158f230 00c1f778 00000000 00000000
>>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>>> GPR24: afb72fc8 00000000 00000001 a5205f30 afb733dc 00000000 a6b85ff4 a5205eb0
>>>>> NIP [a69d6cb0] 0xa69d6cb0
>>>>> LR [a69d6c3c] 0xa69d6c3c
>>>>> --- interrupt: c00
>>>>> Instruction dump:
>>>>> 7cdb3378 93810020 7cbc2b78 93a10024 7c9d2378 93e1002c 7d3f4b78 4800d629
>>>>> 817e0084 931e0088 69690002 5529fffe <0f090000> 69694000 552997fe 0f090000
>>>>> ---[ end trace c66c6c3c44806276 ]---
>>>>>
>>>
>>> Getting a BUG at arch/powerpc/kernel/interrupt.c:49 meaning MSR_RI is not set, but the c00 interrupt
>>> frame shows MSR_RI properly set, so what ?
>>
>> Could the stack be correct but regs pointer incorrect?
>>
>> Instruction dump is
>>
>> 0: 78 33 db 7c mr r27,r6
>> 4: 20 00 81 93 stw r28,32(r1)
>> 8: 78 2b bc 7c mr r28,r5
>> c: 24 00 a1 93 stw r29,36(r1)
>> 10: 78 23 9d 7c mr r29,r4
>> 14: 2c 00 e1 93 stw r31,44(r1)
>> 18: 78 4b 3f 7d mr r31,r9
>> 1c: 29 d6 00 48 bl 0xd644
>> 20: 84 00 7e 81 lwz r11,132(r30)
>> 24: 88 00 1e 93 stw r24,136(r30)
>> 28: 02 00 69 69 xori r9,r11,2
>> 2c: fe ff 29 55 rlwinm r9,r9,31,31,31
>> 30: 00 00 09 0f twnei r9,0
>> 34: 00 40 69 69 xori r9,r11,16384
>> 38: fe 97 29 55 rlwinm r9,r9,18,31,31
>> 3c: 00 00 09 0f twnei r9,0
>>
>> regs->msr is in r11 == 0xce030005 so some kernel address?
>>
>> r1 == 0xe2f75f00
>> r30 == 0xe2f75f40
>>
>> I think that matches if the function allocates 48 bytes of stack.
>> STACK_FRAME_OVERHEAD is 16, so the difference would be 0x40 in that
>> case. Seems okay.
>>
>> I'm not sure then. Can you get a hash fault interrupt come in here
>> because of the vmap stack access and clobber r11? Hmm...
>>
>> fast_hash_page_return:
>> andis. r10, r9, SRR1_ISI_NOPT@h /* Set on ISI, cleared on DSI */
>>
>> Is that really right? DSI can set this bit for NOHPTE as well no?
>
> On DSI, the error bits are in DSISR while they are in SRR1 on ISI.
>
> r9 is supposed to contain SRR1 in both cases. Powerpc 32 bits programming manual explicitely says
> that bits 1-4 and 10-15 of SRR1 are cleared on DSI.
Ah right, I had in mind it was DSISR on DSI and SRR1 on ISI because
we put them together early on 64s. Can't think of anything else at the
moment.
Thanks,
Nick
^ permalink raw reply
* [PATCH] powerpc/kprobes: Fix kprobe Oops happens in booke
From: Pu Lehui @ 2021-08-04 14:37 UTC (permalink / raw)
To: oleg, mpe, benh, paulus, naveen.n.rao, mhiramat, christophe.leroy,
peterz, npiggin, ruscur
Cc: zhangjinhao2, xukuohai, linuxppc-dev, linux-kernel, pulehui
When using kprobe on powerpc booke series processor, Oops happens
as show bellow:
[ 35.861352] Oops: Exception in kernel mode, sig: 5 [#1]
[ 35.861676] BE PAGE_SIZE=4K SMP NR_CPUS=24 QEMU e500
[ 35.861905] Modules linked in:
[ 35.862144] CPU: 0 PID: 76 Comm: sh Not tainted 5.14.0-rc3-00060-g7e96bf476270 #18
[ 35.862610] NIP: c0b96470 LR: c00107b4 CTR: c0161c80
[ 35.862805] REGS: c387fe70 TRAP: 0700 Not tainted (5.14.0-rc3-00060-g7e96bf476270)
[ 35.863198] MSR: 00029002 <CE,EE,ME> CR: 24022824 XER: 20000000
[ 35.863577]
[ 35.863577] GPR00: c0015218 c387ff20 c313e300 c387ff50 00000004 40000002 40000000 0a1a2cce
[ 35.863577] GPR08: 00000000 00000004 00000000 59764000 24022422 102490c2 00000000 00000000
[ 35.863577] GPR16: 00000000 00000000 00000040 10240000 10240000 10240000 10240000 10220000
[ 35.863577] GPR24: ffffffff 10240000 00000000 00000000 bfc655e8 00000800 c387ff50 00000000
[ 35.865367] NIP [c0b96470] schedule+0x0/0x130
[ 35.865606] LR [c00107b4] interrupt_exit_user_prepare_main+0xf4/0x100
[ 35.865974] Call Trace:
[ 35.866142] [c387ff20] [c0053224] irq_exit+0x114/0x120 (unreliable)
[ 35.866472] [c387ff40] [c0015218] interrupt_return+0x14/0x13c
[ 35.866728] --- interrupt: 900 at 0x100af3dc
[ 35.866963] NIP: 100af3dc LR: 100de020 CTR: 00000000
[ 35.867177] REGS: c387ff50 TRAP: 0900 Not tainted (5.14.0-rc3-00060-g7e96bf476270)
[ 35.867488] MSR: 0002f902 <CE,EE,PR,FP,ME> CR: 20022422 XER: 20000000
[ 35.867808]
[ 35.867808] GPR00: c001509c bfc65570 1024b4d0 00000000 100de020 20022422 bfc655a8 100af3dc
[ 35.867808] GPR08: 0002f902 00000000 00000000 00000000 72656773 102490c2 00000000 00000000
[ 35.867808] GPR16: 00000000 00000000 00000040 10240000 10240000 10240000 10240000 10220000
[ 35.867808] GPR24: ffffffff 10240000 00000000 00000000 bfc655e8 10245910 ffffffff 00000001
[ 35.869406] NIP [100af3dc] 0x100af3dc
[ 35.869578] LR [100de020] 0x100de020
[ 35.869751] --- interrupt: 900
[ 35.870001] Instruction dump:
[ 35.870283] 40c20010 815e0518 714a0100 41e2fd04 39200000 913e00c0 3b1e0450 4bfffd80
[ 35.870666] 0fe00000 92a10024 4bfff1a9 60000000 <7fe00008> 7c0802a6 93e1001c 7c5f1378
[ 35.871339] ---[ end trace 23ff848139efa9b9 ]---
There is no real mode for booke arch and the MMU translation is
always on. The corresponding MSR_IS/MSR_DS bit in booke is used
to switch the address space, but not for real mode judgment.
Fixes: 21f8b2fa3ca5 ("powerpc/kprobes: Ignore traps that happened in real mode")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
arch/powerpc/include/asm/ptrace.h | 6 ++++++
arch/powerpc/kernel/kprobes.c | 5 +----
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 3e5d470a6155..4aec1a97024b 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -187,6 +187,12 @@ static inline unsigned long frame_pointer(struct pt_regs *regs)
#define user_mode(regs) (((regs)->msr & MSR_PR) != 0)
#endif
+#ifdef CONFIG_BOOKE
+#define real_mode(regs) 0
+#else
+#define real_mode(regs) (!((regs)->msr & MSR_IR) || !((regs)->msr & MSR_DR))
+#endif
+
#define force_successful_syscall_return() \
do { \
set_thread_flag(TIF_NOERROR); \
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index cbc28d1a2e1b..fac9a5974718 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -289,10 +289,7 @@ int kprobe_handler(struct pt_regs *regs)
unsigned int *addr = (unsigned int *)regs->nip;
struct kprobe_ctlblk *kcb;
- if (user_mode(regs))
- return 0;
-
- if (!(regs->msr & MSR_IR) || !(regs->msr & MSR_DR))
+ if (user_mode(regs) || real_mode(regs))
return 0;
/*
--
2.17.1
^ permalink raw reply related
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 14:26 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #4 from Christophe Leroy (christophe.leroy@csgroup.eu) ---
Thanks, unfortunately the info is still not usable because we don't have
function names in the dump:
[ 15.810083] Call Trace:
[ 15.810090] [f2b6dc30] [c1022318] 0xc1022318 (unreliable)
[ 15.810102] [f2b6dc50] [be987860] 0xbe987860
[ 15.810112] [f2b6dc80] [be85074c] 0xbe85074c
[ 15.810121] [f2b6dcb0] [be984484] 0xbe984484
[ 15.810130] [f2b6dcd0] [c037fd8c] 0xc037fd8c
[ 15.810140] [f2b6dd00] [c03e1460] 0xc03e1460
[ 15.810149] [f2b6dd20] [c03e17f0] 0xc03e17f0
[ 15.810158] [f2b6dd40] [c03e18f4] 0xc03e18f4
[ 15.810168] [f2b6dd70] [c03e21f4] 0xc03e21f4
[ 15.810177] [f2b6dd90] [c03deaf4] 0xc03deaf4
[ 15.810187] [f2b6ddc0] [c03e04d8] 0xc03e04d8
[ 15.810196] [f2b6ddf0] [c03e2d6c] 0xc03e2d6c
[ 15.810205] [f2b6de10] [c0006c34] 0xc0006c34
[ 15.810215] [f2b6de80] [c00cb0d4] 0xc00cb0d4
[ 15.810224] [f2b6deb0] [c00cc2d4] 0xc00cc2d4
[ 15.810233] [f2b6df30] [c0017098] 0xc0017098
Looking at the .config, it seems like CONFIG_KALLSYMS is missing.
Can you retry with CONFIG_KALLSYMS ?
Thanks
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH] powerpc/32: Fix critical and debug interrupts on BOOKE
From: Radu Rendec @ 2021-08-04 14:01 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <c9f3a3e2-e5ed-6074-b700-99911d925012@csgroup.eu>
Hi Christophe,
On Wed, 2021-08-04 at 07:52 +0200, Christophe Leroy wrote:
> Le 07/07/2021 à 07:55, Christophe Leroy a écrit :
> > 32 bits BOOKE have special interrupts for debug and other
> > critical events.
>
> Were you able to test this patch ?
I tested it three weeks ago and it works like a charm!
I'm so sorry I forgot to let you know. I got distracted testing the
old ptrace() problem. In fact, I wouldn't have been able to test that
if your interrupts patch hadn't been working.
Thanks,
Radu
> > When handling those interrupts, dedicated registers are saved
> > in the stack frame in addition to the standard registers, leading
> > to a shift of the pt_regs struct.
> >
> > Since commit db297c3b07af ("powerpc/32: Don't save thread.regs on
> > interrupt entry"), the pt_regs struct is expected to be at the
> > same place all the time.
> >
> > Instead of handling a special struct in addition to pt_regs, just
> > add those special registers to struct pt_regs.
> >
> > Reported-by: Radu Rendec <radu.rendec@gmail.com>
> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > Fixes: db297c3b07af ("powerpc/32: Don't save thread.regs on
> > interrupt entry")
> > Cc: stable@vger.kernel.org
> > ---
> > arch/powerpc/include/asm/ptrace.h | 16 ++++++++++++++++
> > arch/powerpc/kernel/asm-offsets.c | 31
> > ++++++++++++++-----------------
> > arch/powerpc/kernel/head_booke.h | 27
> > +++------------------------
> > 3 files changed, 33 insertions(+), 41 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h
> > b/arch/powerpc/include/asm/ptrace.h
> > index 3e5d470a6155..14422e851494 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -70,6 +70,22 @@ struct pt_regs
> > unsigned long __pad[4]; /* Maintain 16 byte
> > interrupt stack alignment */
> > };
> > #endif
> > +#if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE)
> > + struct { /* Must be a multiple of 16 bytes */
> > + unsigned long mas0;
> > + unsigned long mas1;
> > + unsigned long mas2;
> > + unsigned long mas3;
> > + unsigned long mas6;
> > + unsigned long mas7;
> > + unsigned long srr0;
> > + unsigned long srr1;
> > + unsigned long csrr0;
> > + unsigned long csrr1;
> > + unsigned long dsrr0;
> > + unsigned long dsrr1;
> > + };
> > +#endif
> > };
> > #endif
> >
> > diff --git a/arch/powerpc/kernel/asm-offsets.c
> > b/arch/powerpc/kernel/asm-offsets.c
> > index a47eefa09bcb..5bee245d832b 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -309,24 +309,21 @@ int main(void)
> > STACK_PT_REGS_OFFSET(STACK_REGS_IAMR, iamr);
> > #endif
> >
> > -#if defined(CONFIG_PPC32)
> > -#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> > - DEFINE(EXC_LVL_SIZE, STACK_EXC_LVL_FRAME_SIZE);
> > - DEFINE(MAS0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas0));
> > +#if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE)
> > + STACK_PT_REGS_OFFSET(MAS0, mas0);
> > /* we overload MMUCR for 44x on MAS0 since they are
> > mutually exclusive */
> > - DEFINE(MMUCR, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas0));
> > - DEFINE(MAS1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas1));
> > - DEFINE(MAS2, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas2));
> > - DEFINE(MAS3, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas3));
> > - DEFINE(MAS6, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas6));
> > - DEFINE(MAS7, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas7));
> > - DEFINE(_SRR0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, srr0));
> > - DEFINE(_SRR1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, srr1));
> > - DEFINE(_CSRR0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, csrr0));
> > - DEFINE(_CSRR1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, csrr1));
> > - DEFINE(_DSRR0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, dsrr0));
> > - DEFINE(_DSRR1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, dsrr1));
> > -#endif
> > + STACK_PT_REGS_OFFSET(MMUCR, mas0);
> > + STACK_PT_REGS_OFFSET(MAS1, mas1);
> > + STACK_PT_REGS_OFFSET(MAS2, mas2);
> > + STACK_PT_REGS_OFFSET(MAS3, mas3);
> > + STACK_PT_REGS_OFFSET(MAS6, mas6);
> > + STACK_PT_REGS_OFFSET(MAS7, mas7);
> > + STACK_PT_REGS_OFFSET(_SRR0, srr0);
> > + STACK_PT_REGS_OFFSET(_SRR1, srr1);
> > + STACK_PT_REGS_OFFSET(_CSRR0, csrr0);
> > + STACK_PT_REGS_OFFSET(_CSRR1, csrr1);
> > + STACK_PT_REGS_OFFSET(_DSRR0, dsrr0);
> > + STACK_PT_REGS_OFFSET(_DSRR1, dsrr1);
> > #endif
> >
> > /* About the CPU features table */
> > diff --git a/arch/powerpc/kernel/head_booke.h
> > b/arch/powerpc/kernel/head_booke.h
> > index 87b806e8eded..e5503420b6c6 100644
> > --- a/arch/powerpc/kernel/head_booke.h
> > +++ b/arch/powerpc/kernel/head_booke.h
> > @@ -168,20 +168,18 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_EMB_HV)
> > /* only on e500mc */
> > #define DBG_STACK_BASE dbgirq_ctx
> >
> > -#define EXC_LVL_FRAME_OVERHEAD (THREAD_SIZE - INT_FRAME_SIZE -
> > EXC_LVL_SIZE)
> > -
> > #ifdef CONFIG_SMP
> > #define BOOKE_LOAD_EXC_LEVEL_STACK(level) \
> > mfspr r8,SPRN_PIR; \
> > slwi r8,r8,2; \
> > addis r8,r8,level##_STACK_BASE@ha; \
> > lwz r8,level##_STACK_BASE@l(r8); \
> > - addi r8,r8,EXC_LVL_FRAME_OVERHEAD;
> > + addi r8,r8,THREAD_SIZE - INT_FRAME_SIZE;
> > #else
> > #define BOOKE_LOAD_EXC_LEVEL_STACK(level) \
> > lis r8,level##_STACK_BASE@ha; \
> > lwz r8,level##_STACK_BASE@l(r8); \
> > - addi r8,r8,EXC_LVL_FRAME_OVERHEAD;
> > + addi r8,r8,THREAD_SIZE - INT_FRAME_SIZE;
> > #endif
> >
> > /*
> > @@ -208,7 +206,7 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_EMB_HV)
> > mtmsr r11; \
> > mfspr r11,SPRN_SPRG_THREAD; /* if from user, start at
> > top of */\
> > lwz r11, TASK_STACK - THREAD(r11); /* this thread's
> > kernel stack */\
> > - addi r11,r11,EXC_LVL_FRAME_OVERHEAD; /* allocate stack
> > frame */\
> > + addi r11,r11,THREAD_SIZE - INT_FRAME_SIZE; /* allocate
> > stack frame */\
> > beq 1f;
> > \
> > /* COMING FROM USER MODE
> > */ \
> > stw r9,_CCR(r11); /* save
> > CR */\
> > @@ -516,24 +514,5 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_EMB_HV)
> > bl kernel_fp_unavailable_exception;
> > \
> > b interrupt_return
> >
> > -#else /* __ASSEMBLY__ */
> > -struct exception_regs {
> > - unsigned long mas0;
> > - unsigned long mas1;
> > - unsigned long mas2;
> > - unsigned long mas3;
> > - unsigned long mas6;
> > - unsigned long mas7;
> > - unsigned long srr0;
> > - unsigned long srr1;
> > - unsigned long csrr0;
> > - unsigned long csrr1;
> > - unsigned long dsrr0;
> > - unsigned long dsrr1;
> > -};
> > -
> > -/* ensure this structure is always sized to a multiple of the
> > stack alignment */
> > -#define STACK_EXC_LVL_FRAME_SIZE ALIGN(sizeof (struct
> > exception_regs), 16)
> > -
> > #endif /* __ASSEMBLY__ */
> > #endif /* __HEAD_BOOKE_H__ */
> >
^ permalink raw reply
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 13:30 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #3 from Elimar Riesebieter (riesebie@lxtec.de) ---
Created attachment 298197
--> https://bugzilla.kernel.org/attachment.cgi?id=298197&action=edit
.config
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 13:29 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #2 from Elimar Riesebieter (riesebie@lxtec.de) ---
Created attachment 298195
--> https://bugzilla.kernel.org/attachment.cgi?id=298195&action=edit
dmesg dump
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Christophe Leroy @ 2021-08-04 13:28 UTC (permalink / raw)
To: Nicholas Piggin
Cc: userm57, Finn Thain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <1628068469.gv4bl1fw7w.astroid@bobo.none>
Le 04/08/2021 à 13:36, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of August 4, 2021 4:21 pm:
>> Hi Nic,
>>
>> I think I'll need your help on that one.
>>
>> Le 04/08/2021 à 08:07, Christophe Leroy a écrit :
>>>
>>>
>>> Le 04/08/2021 à 06:04, Finn Thain a écrit :
>
> Hi Finn!
>
>>>> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>>>>
>> ...
>>>>
>>>> ------------[ cut here ]------------
>>>> kernel BUG at arch/powerpc/kernel/interrupt.c:49!
>>>> Oops: Exception in kernel mode, sig: 5 [#1]
>>>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
>>>> Modules linked in:
>>>> CPU: 0 PID: 1859 Comm: xfce4-session Not tainted 5.13.0-pmac-VMAP #10
>>>> NIP: c0011474 LR: c0011464 CTR: 00000000
>>>> REGS: e2f75e40 TRAP: 0700 Not tainted (5.13.0-pmac-VMAP)
>>>> MSR: 00021032 <ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>
>>>> GPR00: c001604c e2f75f00 ca284a60 00000000 00000000 a5205eb0 00000008 00000020
>>>> GPR08: ffffffc0 00000001 501200d9 ce030005 ca285010 00c1f778 00000000 00000000
>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>> GPR24: 00000000 ffffffc0 00000020 00000008 a5205eb0 00000000 e2f75f40 000000ae
>>>> NIP [c0011474] system_call_exception+0x60/0x164
>>>> LR [c0011464] system_call_exception+0x50/0x164
>>>> Call Trace:
>>>> [e2f75f00] [00009000] 0x9000 (unreliable)
>>>> [e2f75f30] [c001604c] ret_from_syscall+0x0/0x28
>>>> --- interrupt: c00 at 0xa69d6cb0
>>>> NIP: a69d6cb0 LR: a69d6c3c CTR: 00000000
>>>> REGS: e2f75f40 TRAP: 0c00 Not tainted (5.13.0-pmac-VMAP)
>>>> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>
>>>> GPR00: 000000ae a5205de0 a5687ca0 00000000 00000000 a5205eb0 00000008 00000020
>>>> GPR08: ffffffc0 401201ea 401200d9 ffffffff c158f230 00c1f778 00000000 00000000
>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>> GPR24: afb72fc8 00000000 00000001 a5205f30 afb733dc 00000000 a6b85ff4 a5205eb0
>>>> NIP [a69d6cb0] 0xa69d6cb0
>>>> LR [a69d6c3c] 0xa69d6c3c
>>>> --- interrupt: c00
>>>> Instruction dump:
>>>> 7cdb3378 93810020 7cbc2b78 93a10024 7c9d2378 93e1002c 7d3f4b78 4800d629
>>>> 817e0084 931e0088 69690002 5529fffe <0f090000> 69694000 552997fe 0f090000
>>>> ---[ end trace c66c6c3c44806276 ]---
>>>>
>>
>> Getting a BUG at arch/powerpc/kernel/interrupt.c:49 meaning MSR_RI is not set, but the c00 interrupt
>> frame shows MSR_RI properly set, so what ?
>
> Could the stack be correct but regs pointer incorrect?
>
> Instruction dump is
>
> 0: 78 33 db 7c mr r27,r6
> 4: 20 00 81 93 stw r28,32(r1)
> 8: 78 2b bc 7c mr r28,r5
> c: 24 00 a1 93 stw r29,36(r1)
> 10: 78 23 9d 7c mr r29,r4
> 14: 2c 00 e1 93 stw r31,44(r1)
> 18: 78 4b 3f 7d mr r31,r9
> 1c: 29 d6 00 48 bl 0xd644
> 20: 84 00 7e 81 lwz r11,132(r30)
> 24: 88 00 1e 93 stw r24,136(r30)
> 28: 02 00 69 69 xori r9,r11,2
> 2c: fe ff 29 55 rlwinm r9,r9,31,31,31
> 30: 00 00 09 0f twnei r9,0
> 34: 00 40 69 69 xori r9,r11,16384
> 38: fe 97 29 55 rlwinm r9,r9,18,31,31
> 3c: 00 00 09 0f twnei r9,0
>
> regs->msr is in r11 == 0xce030005 so some kernel address?
>
> r1 == 0xe2f75f00
> r30 == 0xe2f75f40
>
> I think that matches if the function allocates 48 bytes of stack.
> STACK_FRAME_OVERHEAD is 16, so the difference would be 0x40 in that
> case. Seems okay.
>
> I'm not sure then. Can you get a hash fault interrupt come in here
> because of the vmap stack access and clobber r11? Hmm...
>
> fast_hash_page_return:
> andis. r10, r9, SRR1_ISI_NOPT@h /* Set on ISI, cleared on DSI */
>
> Is that really right? DSI can set this bit for NOHPTE as well no?
On DSI, the error bits are in DSISR while they are in SRR1 on ISI.
r9 is supposed to contain SRR1 in both cases. Powerpc 32 bits programming manual explicitely says
that bits 1-4 and 10-15 of SRR1 are cleared on DSI.
Thanks,
Christophe
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Petr Mladek @ 2021-08-04 12:31 UTC (permalink / raw)
To: John Ogness
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Daniel Thompson, Bhaskar Chowdhury, x86,
Ingo Molnar, kgdb-bugreport, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <87tuk635rb.fsf@jogness.linutronix.de>
On Tue 2021-08-03 17:36:32, John Ogness wrote:
> On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> >> during cpu roundup. This will conflict with the printk cpulock.
> >
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 3d0c933937b4..1b546e117f10 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -214,6 +215,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> >> #ifdef CONFIG_SMP
> >> static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
> >> static atomic_t printk_cpulock_nested = ATOMIC_INIT(0);
> >> +static unsigned int kgdb_cpu = -1;
> >
> > Is this the flag to provoke retriggering? It appears to be a write-only
> > variable (at least in this patch). How is it consumed?
>
> Critical catch! Thank you. I am quite unhappy to see these hunks were
> accidentally dropped when generating this series.
>
> @@ -3673,6 +3675,9 @@ EXPORT_SYMBOL(__printk_cpu_trylock);
> */
> void __printk_cpu_unlock(void)
> {
> + bool trigger_kgdb = false;
> + unsigned int cpu;
> +
> if (atomic_read(&printk_cpulock_nested)) {
> atomic_dec(&printk_cpulock_nested);
> return;
> @@ -3683,6 +3688,12 @@ void __printk_cpu_unlock(void)
> * LMM(__printk_cpu_unlock:A)
> */
>
> + cpu = smp_processor_id();
> + if (kgdb_cpu == cpu) {
> + trigger_kgdb = true;
> + kgdb_cpu = -1;
> + }
Just in case that this approach is used in the end.
This code looks racy. kgdb_roundup_delay() seems to be called in NMI
context. NMI might happen at this point and set kgdb_cpu after
it was checked.
I am afraid that it won't be easy to make this safe using a single
global variable. A solution might be a per-CPU variable set
by kgdb_roundup_delay() when it owns printk_cpu_lock.
__printk_cpu_unlock() would call kgdb_roundup_cpu(cpu) when
the variable is set.
Nit: The name "kgdb_cpu" is too generic. It is not clear what is
so special about this CPU. I would call the per-CPU variable
"kgdb_delayed_roundup" or so.
Best Regards,
Petr
> /*
> * Guarantee loads and stores from this CPU when it was the
> * lock owner are visible to the next lock owner. This pairs
> @@ -3703,6 +3714,21 @@ void __printk_cpu_unlock(void)
> */
> atomic_set_release(&printk_cpulock_owner,
> -1); /* LMM(__printk_cpu_unlock:B) */
> +
> + if (trigger_kgdb) {
> + pr_warn("re-triggering kgdb roundup for CPU#%d\n", cpu);
> + kgdb_roundup_cpu(cpu);
> + }
> }
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Petr Mladek @ 2021-08-04 12:12 UTC (permalink / raw)
To: Daniel Thompson
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Bhaskar Chowdhury, x86, Ingo Molnar,
kgdb-bugreport, John Ogness, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <20210804113159.lsnoyylifg6v5i35@maple.lan>
On Wed 2021-08-04 12:31:59, Daniel Thompson wrote:
> On Tue, Aug 03, 2021 at 05:36:32PM +0206, John Ogness wrote:
> > On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> > >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> > >> during cpu roundup. This will conflict with the printk cpulock.
> > >
> > > When the full vision is realized what will be the purpose of the printk
> > > cpulock?
> > >
> > > I'm asking largely because it's current role is actively unhelpful
> > > w.r.t. kdb. It is possible that cautious use of in_dbg_master() might
> > > be a better (and safer) solution. However it sounds like there is a
> > > larger role planned for the printk cpulock...
> >
> > The printk cpulock is used as a synchronization mechanism for
> > implementing atomic consoles, which need to be able to safely interrupt
> > the console write() activity at any time and immediately continue with
> > their own printing. The ultimate goal is to move all console printing
> > into per-console dedicated kthreads, so the primary function of the
> > printk cpulock is really to immediately _stop_ the CPU/kthread
> > performing write() in order to allow write_atomic() (from any context on
> > any CPU) to safely and reliably take over.
>
> I see.
>
> Is there any mileage in allowing in_dbg_master() to suppress taking
> the console lock?
>
> There's a couple of reasons to worry about the current approach.
>
> The first is that we don't want this code to trigger in the case when
> kgdb is enabled and kdb is not since it is only kdb (a self-hosted
> debugger) than uses the consoles. This case is relatively trivial to
> address since we can rename it kdb_roundup_delay() and alter the way it
> is conditionally compiled.
>
> The second is more of a problem however. kdb will only call into the
> console code from the debug master. By default this is the CPU that
> takes the debug trap so initial prints will work fine. However it is
> possible to switch to a different master (so we can read per-CPU
> registers and things like that). This will result in one of the CPUs
> that did the IPI round up calling into console code and this is unsafe
> in that instance.
>
> There are a couple of tricks we could adopt to work around this but
> given the slightly odd calling context for kdb (all CPUs quiesced, no
> log interleaving possible) it sounds like it would remain safe to
> bypass the lock if in_dbg_master() is true.
>
> Bypassing an inconvenient lock might sound icky but:
>
> 1. If the lock is not owned by any CPU then what kdb will do is safe.
>
> 2. If the lock is owned by any CPU then we have quiesced it anyway
> and this makes is safe for the owning CPU to share its ownership
> (since it isn't much different to recursive acquisition on a single
> CPU)
I think about the following:
void kgdb_roundup_cpus(void)
{
__printk_cpu_lock();
__kgdb_roundup_cpus();
}
, where __printk_cpu_lock() waits/takes printk_cpu_lock()
__kgdb_roundup_cpus() is the original kgdb_roundup_cpus();
The idea is that kgdb_roundup_cpus() caller takes the printk_cpu lock.
The owner will be well defined.
As a result any other CPU will not be able to take the printk_cpu lock
as long as it is owned by the kgdb lock. But as you say, kgdb will
make sure that everything is serialized at this stage. So that
the original raw_printk_cpu_lock_irqsave() might just disable
IRQs when called under debugger.
Does it make any sense?
I have to say that it is a bit hairy. But it looks slightly better
than the delayed/repeated IPI proposed by this patch.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Nicholas Piggin @ 2021-08-04 11:36 UTC (permalink / raw)
To: Christophe Leroy
Cc: userm57, Finn Thain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <8fb08f68-ed01-65f9-fb9e-66abf2b18a00@csgroup.eu>
Excerpts from Christophe Leroy's message of August 4, 2021 4:21 pm:
> Hi Nic,
>
> I think I'll need your help on that one.
>
> Le 04/08/2021 à 08:07, Christophe Leroy a écrit :
>>
>>
>> Le 04/08/2021 à 06:04, Finn Thain a écrit :
Hi Finn!
>>> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>>>
> ...
>>>
>>> ------------[ cut here ]------------
>>> kernel BUG at arch/powerpc/kernel/interrupt.c:49!
>>> Oops: Exception in kernel mode, sig: 5 [#1]
>>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
>>> Modules linked in:
>>> CPU: 0 PID: 1859 Comm: xfce4-session Not tainted 5.13.0-pmac-VMAP #10
>>> NIP: c0011474 LR: c0011464 CTR: 00000000
>>> REGS: e2f75e40 TRAP: 0700 Not tainted (5.13.0-pmac-VMAP)
>>> MSR: 00021032 <ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>
>>> GPR00: c001604c e2f75f00 ca284a60 00000000 00000000 a5205eb0 00000008 00000020
>>> GPR08: ffffffc0 00000001 501200d9 ce030005 ca285010 00c1f778 00000000 00000000
>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>> GPR24: 00000000 ffffffc0 00000020 00000008 a5205eb0 00000000 e2f75f40 000000ae
>>> NIP [c0011474] system_call_exception+0x60/0x164
>>> LR [c0011464] system_call_exception+0x50/0x164
>>> Call Trace:
>>> [e2f75f00] [00009000] 0x9000 (unreliable)
>>> [e2f75f30] [c001604c] ret_from_syscall+0x0/0x28
>>> --- interrupt: c00 at 0xa69d6cb0
>>> NIP: a69d6cb0 LR: a69d6c3c CTR: 00000000
>>> REGS: e2f75f40 TRAP: 0c00 Not tainted (5.13.0-pmac-VMAP)
>>> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>
>>> GPR00: 000000ae a5205de0 a5687ca0 00000000 00000000 a5205eb0 00000008 00000020
>>> GPR08: ffffffc0 401201ea 401200d9 ffffffff c158f230 00c1f778 00000000 00000000
>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>> GPR24: afb72fc8 00000000 00000001 a5205f30 afb733dc 00000000 a6b85ff4 a5205eb0
>>> NIP [a69d6cb0] 0xa69d6cb0
>>> LR [a69d6c3c] 0xa69d6c3c
>>> --- interrupt: c00
>>> Instruction dump:
>>> 7cdb3378 93810020 7cbc2b78 93a10024 7c9d2378 93e1002c 7d3f4b78 4800d629
>>> 817e0084 931e0088 69690002 5529fffe <0f090000> 69694000 552997fe 0f090000
>>> ---[ end trace c66c6c3c44806276 ]---
>>>
>
> Getting a BUG at arch/powerpc/kernel/interrupt.c:49 meaning MSR_RI is not set, but the c00 interrupt
> frame shows MSR_RI properly set, so what ?
Could the stack be correct but regs pointer incorrect?
Instruction dump is
0: 78 33 db 7c mr r27,r6
4: 20 00 81 93 stw r28,32(r1)
8: 78 2b bc 7c mr r28,r5
c: 24 00 a1 93 stw r29,36(r1)
10: 78 23 9d 7c mr r29,r4
14: 2c 00 e1 93 stw r31,44(r1)
18: 78 4b 3f 7d mr r31,r9
1c: 29 d6 00 48 bl 0xd644
20: 84 00 7e 81 lwz r11,132(r30)
24: 88 00 1e 93 stw r24,136(r30)
28: 02 00 69 69 xori r9,r11,2
2c: fe ff 29 55 rlwinm r9,r9,31,31,31
30: 00 00 09 0f twnei r9,0
34: 00 40 69 69 xori r9,r11,16384
38: fe 97 29 55 rlwinm r9,r9,18,31,31
3c: 00 00 09 0f twnei r9,0
regs->msr is in r11 == 0xce030005 so some kernel address?
r1 == 0xe2f75f00
r30 == 0xe2f75f40
I think that matches if the function allocates 48 bytes of stack.
STACK_FRAME_OVERHEAD is 16, so the difference would be 0x40 in that
case. Seems okay.
I'm not sure then. Can you get a hash fault interrupt come in here
because of the vmap stack access and clobber r11? Hmm...
fast_hash_page_return:
andis. r10, r9, SRR1_ISI_NOPT@h /* Set on ISI, cleared on DSI */
Is that really right? DSI can set this bit for NOHPTE as well no?
That'd do it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Daniel Thompson @ 2021-08-04 11:31 UTC (permalink / raw)
To: John Ogness
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Bhaskar Chowdhury, x86, Ingo Molnar,
kgdb-bugreport, Petr Mladek, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <87tuk635rb.fsf@jogness.linutronix.de>
On Tue, Aug 03, 2021 at 05:36:32PM +0206, John Ogness wrote:
> On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> >> during cpu roundup. This will conflict with the printk cpulock.
> >
> > When the full vision is realized what will be the purpose of the printk
> > cpulock?
> >
> > I'm asking largely because it's current role is actively unhelpful
> > w.r.t. kdb. It is possible that cautious use of in_dbg_master() might
> > be a better (and safer) solution. However it sounds like there is a
> > larger role planned for the printk cpulock...
>
> The printk cpulock is used as a synchronization mechanism for
> implementing atomic consoles, which need to be able to safely interrupt
> the console write() activity at any time and immediately continue with
> their own printing. The ultimate goal is to move all console printing
> into per-console dedicated kthreads, so the primary function of the
> printk cpulock is really to immediately _stop_ the CPU/kthread
> performing write() in order to allow write_atomic() (from any context on
> any CPU) to safely and reliably take over.
I see.
Is there any mileage in allowing in_dbg_master() to suppress taking
the console lock?
There's a couple of reasons to worry about the current approach.
The first is that we don't want this code to trigger in the case when
kgdb is enabled and kdb is not since it is only kdb (a self-hosted
debugger) than uses the consoles. This case is relatively trivial to
address since we can rename it kdb_roundup_delay() and alter the way it
is conditionally compiled.
The second is more of a problem however. kdb will only call into the
console code from the debug master. By default this is the CPU that
takes the debug trap so initial prints will work fine. However it is
possible to switch to a different master (so we can read per-CPU
registers and things like that). This will result in one of the CPUs
that did the IPI round up calling into console code and this is unsafe
in that instance.
There are a couple of tricks we could adopt to work around this but
given the slightly odd calling context for kdb (all CPUs quiesced, no
log interleaving possible) it sounds like it would remain safe to
bypass the lock if in_dbg_master() is true.
Bypassing an inconvenient lock might sound icky but:
1. If the lock is not owned by any CPU then what kdb will do is safe.
2. If the lock is owned by any CPU then we have quiesced it anyway
and this makes is safe for the owning CPU to share its ownership
(since it isn't much different to recursive acquisition on a single
CPU)
> Atomic consoles are actually quite similar to the kgdb_io ops. For
> example, comparing:
>
> serial8250_console_write_atomic() + serial8250_console_putchar_locked()
>
> with
>
> serial8250_put_poll_char()
>
> The difference is that serial8250_console_write_atomic() is line-based
> and synchronizing with serial8250_console_write() so that if the kernel
> crashes while outputing to the console, write() can be interrupted by
> write_atomic() and cleanly formatted crash data can be output.
>
> Also serial8250_put_poll_char() is calling into __pm_runtime_resume(),
> which includes a spinlock and possibly sleeping. This would not be
> acceptable for atomic consoles.
spinlocks aren't allowed in polled I/O either.
However IIRC there is a rather nasty trick being played here to allow
code sharing. I believe there was a deliberate unbalanced resume in the
poll_init() function that results (again IIRC) in the PM calls in
poll_char() becoming nothing but atomic add and subtract (e.g. enabling
polled I/O effectively suppresses PM activity).
Daniel.
> Although, as Andy pointed out [0], I
> will need to figure out how to deal with suspended consoles. Or just
> implement a policy that registered atomic consoles may never be
> suspended.
>
> I had not considered merging kgdb_io ops with atomic console ops. But
> now that I look at it more closely, there may be some useful overlap. I
> will consider this. Thank you for this idea.
>
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 3d0c933937b4..1b546e117f10 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -214,6 +215,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> >> #ifdef CONFIG_SMP
> >> static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
> >> static atomic_t printk_cpulock_nested = ATOMIC_INIT(0);
> >> +static unsigned int kgdb_cpu = -1;
> >
> > Is this the flag to provoke retriggering? It appears to be a write-only
> > variable (at least in this patch). How is it consumed?
>
> Critical catch! Thank you. I am quite unhappy to see these hunks were
> accidentally dropped when generating this series.
>
> @@ -3673,6 +3675,9 @@ EXPORT_SYMBOL(__printk_cpu_trylock);
> */
> void __printk_cpu_unlock(void)
> {
> + bool trigger_kgdb = false;
> + unsigned int cpu;
> +
> if (atomic_read(&printk_cpulock_nested)) {
> atomic_dec(&printk_cpulock_nested);
> return;
> @@ -3683,6 +3688,12 @@ void __printk_cpu_unlock(void)
> * LMM(__printk_cpu_unlock:A)
> */
>
> + cpu = smp_processor_id();
> + if (kgdb_cpu == cpu) {
> + trigger_kgdb = true;
> + kgdb_cpu = -1;
> + }
> +
> /*
> * Guarantee loads and stores from this CPU when it was the
> * lock owner are visible to the next lock owner. This pairs
> @@ -3703,6 +3714,21 @@ void __printk_cpu_unlock(void)
> */
> atomic_set_release(&printk_cpulock_owner,
> -1); /* LMM(__printk_cpu_unlock:B) */
> +
> + if (trigger_kgdb) {
> + pr_warn("re-triggering kgdb roundup for CPU#%d\n", cpu);
> + kgdb_roundup_cpu(cpu);
> + }
> }
> EXPORT_SYMBOL(__printk_cpu_unlock);
>
> John Ogness
>
> [0] https://lore.kernel.org/lkml/YQlKAeXS9MPmE284@smile.fi.intel.com
^ permalink raw reply
* Re: [PATCH v2 1/2] sched/topology: Skip updating masks for non-online nodes
From: Valentin Schneider @ 2021-08-04 10:20 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Rik van Riel,
Peter Zijlstra, linuxppc-dev, Geetika Moolchandani, LKML,
Dietmar Eggemann, Thomas Gleixner, Laurent Dufour, Mel Gorman,
Ingo Molnar
In-Reply-To: <20210804100155.GE4072958@linux.vnet.ibm.com>
On 04/08/21 15:31, Srikar Dronamraju wrote:
> * Srikar Dronamraju <srikar@linux.vnet.ibm.com> [2021-07-23 20:09:14]:
>
>> * Valentin Schneider <valentin.schneider@arm.com> [2021-07-13 17:32:14]:
>>
>> > On 12/07/21 18:18, Srikar Dronamraju wrote:
>> > > Hi Valentin,
>> > >
>> > >> On 01/07/21 09:45, Srikar Dronamraju wrote:
>> > >> > @@ -1891,12 +1894,30 @@ void sched_init_numa(void)
>> > >> > void sched_domains_numa_masks_set(unsigned int cpu)
>> > >> > {
>> > >
>
> Hey Valentin / Peter
>
> Did you get a chance to look at this?
>
Barely, I wanted to set some time aside to stare at this and have been
failing miserably. Let me bump it up my todolist, I'll get to it before the
end of the week.
> --
> Thanks and Regards
> Srikar Dronamraju
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox