Skip to content

drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc #6414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xiaoxiang781216 merged 1 commit into from
Jun 13, 2022
Merged

drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc #6414

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 70 additions & 9 deletions drivers/syslog/syslog_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
#include <nuttx/arch.h>
#include <nuttx/kmalloc.h>
#include <nuttx/fs/fs.h>
#include <nuttx/mutex.h>
#include <nuttx/semaphore.h>
#include <nuttx/syslog/syslog.h>
#include <nuttx/compiler.h>

Expand All @@ -54,8 +54,10 @@
*/

#define SYSLOG_OFLAGS (O_WRONLY | O_CREAT | O_APPEND)
#define syslog_dev_takesem(s) nxrmutex_lock(&(s)->sl_lock)
#define syslog_dev_givesem(s) nxrmutex_unlock(&(s)->sl_lock)

/* An invalid thread ID */

#define NO_HOLDER (INVALID_PROCESS_ID)

/****************************************************************************
* Private Types
Expand All @@ -81,7 +83,8 @@ struct syslog_dev_s
uint8_t sl_state; /* See enum syslog_dev_state */
uint8_t sl_oflags; /* Saved open mode (for re-open) */
uint16_t sl_mode; /* Saved open flags (for re-open) */
rmutex_t sl_lock; /* Enforces mutually exclusive access */
sem_t sl_sem; /* Enforces mutually exclusive access */
pid_t sl_holder; /* PID of the thread that holds the semaphore */
struct file sl_file; /* The syslog file structure */
FAR char *sl_devpath; /* Full path to the character device */
};
Expand Down Expand Up @@ -120,6 +123,63 @@ static const uint8_t g_syscrlf[2] =
* Private Functions
****************************************************************************/

/****************************************************************************
* Name: syslog_dev_takesem
****************************************************************************/

static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
Comment thread
xiaoxiang781216 marked this conversation as resolved.
{
pid_t me = getpid();
int ret;

/* Does this thread already hold the semaphore? That could happen if
* we were called recursively, i.e., if the logic kicked off by
* file_write() where to generate more debug output. Return an
* error in that case.
*/

if (syslog_dev->sl_holder == me)
{
/* Return an error (instead of deadlocking) */

return -EWOULDBLOCK;
}

/* Either the semaphore is available or is currently held by another
* thread. Wait for it to become available.
*/

ret = nxsem_wait(&syslog_dev->sl_sem);
if (ret < 0)
{
return ret;
}

/* We hold the semaphore. We can safely mark ourself as the holder
* of the semaphore.
*/

syslog_dev->sl_holder = me;
return OK;
}

/****************************************************************************
* Name: syslog_dev_givesem
****************************************************************************/

static inline void syslog_dev_givesem(FAR struct syslog_dev_s *syslog_dev)
{
#ifdef CONFIG_DEBUG_ASSERTIONS
pid_t me = getpid();
DEBUGASSERT(syslog_dev->sl_holder == me);
#endif

/* Relinquish the semaphore */

syslog_dev->sl_holder = NO_HOLDER;
nxsem_post(&syslog_dev->sl_sem);
}

/****************************************************************************
* Name: syslog_dev_open
*
Expand Down Expand Up @@ -212,8 +272,9 @@ static int syslog_dev_open(FAR struct syslog_dev_s *syslog_dev,

/* The SYSLOG device is open and ready for writing. */

nxrmutex_init(&syslog_dev->sl_lock);
syslog_dev->sl_state = SYSLOG_OPENED;
nxsem_init(&syslog_dev->sl_sem, 0, 1);
syslog_dev->sl_holder = NO_HOLDER;
syslog_dev->sl_state = SYSLOG_OPENED;
return OK;
}

Expand Down Expand Up @@ -242,7 +303,7 @@ static int syslog_dev_open(FAR struct syslog_dev_s *syslog_dev,
* close the device, and set it for later re-opening.
*
* NOTE: That the third case is different. It applies only to the thread
* that currently holds the sl_lock. Other threads should wait.
* that currently holds the sl_sem semaphore. Other threads should wait.
* that is why that case is handled in syslog_semtake().
*
* Input Parameters:
Expand Down Expand Up @@ -291,7 +352,7 @@ static int syslog_dev_outputready(FAR struct syslog_dev_s *syslog_dev)
if (syslog_dev->sl_state == SYSLOG_FAILURE)
{
file_close(&syslog_dev->sl_file);
nxrmutex_destroy(&syslog_dev->sl_lock);
nxsem_destroy(&syslog_dev->sl_sem);

syslog_dev->sl_state = SYSLOG_REOPEN;
}
Expand Down Expand Up @@ -734,7 +795,7 @@ void syslog_dev_uninitialize(FAR struct syslog_channel_s *channel)
syslog_dev->sl_state == SYSLOG_FAILURE)
{
file_close(&syslog_dev->sl_file);
nxrmutex_destroy(&syslog_dev->sl_lock);
nxsem_destroy(&syslog_dev->sl_sem);
}

/* Set the device in UNINITIALIZED state. */
Expand Down